bryanpizzillo
commented
4 months ago For the record, there were a number of issues with the ticket/PR that needed to be addressed:
- [x] The page type was not defined clearly on the ticket, which lead to the code not really defining it well. The ticket has been updated with additional types and clearer rules.
- The original PR submission completely missed "Media" from the original list.
- [x] The logic for determining the
titleparameter for analytics worked, but only under very narrow circumstances. If a link was in a second paragraph after a heading, within the same content block, it would not pick up the heading. - [x] The managed links (only for node) in the WYSIWYG content were hard coded and would most likely cause other content to be linked as new yaml pages were entered. This was not possible, so #4393 was added to handle this.
- [x] The test content (YAML and unit test .dom.ts file) were a ton of content blocks each with only 1 link. The ticket was updated to clearly spell out how many blocks. (Needless to say, when there is 1 link per block it is hard to tell if link counting is working.)
- [x] I did not like how the name of the component was just cgdp-content-block. We should call this cgdp-landing-content-block. This is so that when we start allowing embedded content blocks in WYSIWYG editors, there will not be a name clash.
- IDK if there was anything else from the original reviews/actually testing the code.
monika-jaeger
bennettcc
ODE: https://ncigovcdode779.prod.acquia-sites.com/test/ncids-mini-landing/template Closes #4359