-
Notifications
You must be signed in to change notification settings - Fork 30
(#4804) Add Embed Block Content Block and raw HTML Block to Article #5063
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(#4804) Add Embed Block Content Block and raw HTML Block to Article #5063
Conversation
4106a73 to
e08b448
Compare
|
Reopengin PR; commit had problem with message |
e08b448 to
2b0544e
Compare
|
/test/cgov-article-testing-embedded-raw-html-blocks |
2b0544e to
804d593
Compare
85c575d to
6a9b3b0
Compare
ODE DeploymentCode has been deployed to ODE 1053. |
88e2ff5 to
2deb660
Compare
|
This passes the Raw HTML addition, but we will bring #5065 back into this PR to add both the Content Block and Raw HTML together. This will be rereviewed following the update to the Content Block. |
|
@andyvanavery31 , @erikscottmeyer ,
|
|
Alignment does not work - all options display block as left aligned |
|
@erikscottmeyer , confirmed that icon positioning will be fixed with the Content Block PR. Confirmed that the alignment (None, Left, Center, Right) will not work for Raw HTML Block at the front end. Confirmed with @andyvanavery31 , that the spacing issue and the look and feel of links, buttons are inconsistent with dev-ac as raw HTML block is using legacy html which isn't supported. |
|
Rest looks good |
4656211 to
7431b93
Compare
|
Test page for content block embeds: |
|
@erikscottmeyer Like we discussed the alignment doesn't change no matter which option you choose for content blocks— which is the way this currently functions so that's okay. But it did have some effects to the display in the WYSIYWG: Same for the raw HTML block, but the alignment is even more pronounced on the WYSIYWG:
cc: @andyvanavery31 |
bryanpizzillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes for this PR should not be affecting legacy content. By changing the configurations for insert_block_content the legacy pages (biography, press release, etc) are no longer able to insert External Cards into the WYSIWYG. Additionally you cannot changes the options for existing embedded cards such as on https://ncigovcdode1053.prod.acquia-sites.com/test/biography-feature-card-embed-test-page. (If you click the pencil on any external cards, a console error appears)
7431b93 to
2e797b7
Compare
|
Looks good. Gets the IA checkmark |
d6e67df to
f35a643
Compare
bryanpizzillo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some wonky dependencies being pulled in.
| core_version_requirement: '^10 || ^11' | ||
| dependencies: | ||
| - 'cgov_core:cgov_core' | ||
| - 'cgov_site_section:cgov_site_section' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does content block require site section? It should not.
...s/custom/cgov_site/modules/custom/cgov_core/config/install/filter.format.ncids_full_html.yml
Show resolved
Hide resolved
| package: 'CGov Digital Platform' | ||
| description: 'CGov Core Components (used by the CGov Site install profile)' | ||
| core_version_requirement: '^9.4 || ^10' | ||
| core_version_requirement: '^10 || ^11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file can be reverted.
| package: 'CGov Digital Platform' | ||
| description: 'The Content Block content type' | ||
| core_version_requirement: '^9.4 || ^10' | ||
| core_version_requirement: '^10 || ^11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume features did this?
| display_review: false | ||
| icon: | ||
| uri: 'public://embed_buttons/embed_block.svg' | ||
| data: PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0iVVRGLTgiPz48c3ZnIGlkPSJMYXllcl8xIiB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHZpZXdCb3g9IjAgMCA2MDAgNjAwIj48ZGVmcz48c3R5bGU+LmNscy0xe2ZpbGw6IzMzMztzdHJva2Utd2lkdGg6MHB4O308L3N0eWxlPjwvZGVmcz48cmVjdCBjbGFzcz0iY2xzLTEiIHg9IjQ2IiB5PSI0MC40NSIgd2lkdGg9IjIyNyIgaGVpZ2h0PSI1MjIiIHJ4PSI4IiByeT0iOCIvPjxyZWN0IGNsYXNzPSJjbHMtMSIgeD0iMzEzIiB5PSI0MC40NSIgd2lkdGg9IjI0MiIgaGVpZ2h0PSIxNzIiIHJ4PSI4IiByeT0iOCIvPjxyZWN0IGNsYXNzPSJjbHMtMSIgeD0iMzEzIiB5PSIyNTYuNDUiIHdpZHRoPSIyNDIiIGhlaWdodD0iMzA2IiByeD0iOCIgcnk9IjgiLz48L3N2Zz4= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reference the icon like we do for the other buttons... https://github.com/NCIOCPL/cgov-digital-platform/blob/f35a6438486bbd5b1f6ec8c02b8500704107ba96/docroot/profiles/custom/cgov_site/modules/custom/cgov_embedded_block/config/install/embed.button.insert_block_content.yml
It has something to do with reinstalling or something like that... The module:// reference means it can load from this module.
| package: 'CGov Digital Platform' | ||
| description: 'Embeddable block content support' | ||
| core_version_requirement: '^9.4 || ^10' | ||
| core_version_requirement: '^10 || ^11' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it this is another features feature?
| core_version_requirement: '^9.4 || ^10' | ||
| core_version_requirement: '^10 || ^11' | ||
| dependencies: | ||
| - 'cgov_blog:cgov_blog' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These dependencies are off. We should not be requiring all these cgov_ modules.
f35a643 to
5271807
Compare
|
This PR passes Product Testing again. |
c1114f4 to
9c5e730
Compare
9c5e730 to
515acaf
Compare
|
@erikscottmeyer I am seeing a ton of the following errors when logged as an admin when I view pages clicking from the content list. (So login, and view http://www.devbox/test/biography-image-embed-test-page. I did it by going to the content list and finding the page. |
| * Add theme suggestions for entity embed container. | ||
| */ | ||
| function ncids_trans_theme_suggestions_entity_embed_container_alter(array &$suggestions, array $variables) { | ||
| if (!$variables['element']['entity']['#block_content']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, yeah, so if we embed media or nodes this would break. I assume you want
if (empty($variables['element']['entity']['#block_content'])) {
515acaf to
cf32228
Compare
cf32228 to
52a88dd
Compare
52a88dd to
3eeb769
Compare
|
Confirmed the latest changes are correct and this is still approved. |








Closes #4804
Closes #5065