Page component embedding in WYSIWYG

[Adnan-cds]

To embed Page components with Geolocation fields, we need to convince CKEditor to accept meta tags within span tags.

New Entity embed button for the WYSIWYG. This button triggers a modal dialog to insert a new or existing Page component.

[Adnan-cds]

Hi Dan, I have managed to recover the code for Page component embedding from our old code base. I need to test it further before handing it to you. There are a few manual steps involved such as enabling the entity_embed module, configuring entity embedding within the WYSIWYG, etc. I hope to write all these up next week.

@danchamp @willguv @tom-steel

[Adnan-cds]

Hi Dan, I have added the necessary code and tested it. It works, but embedding localgov_contact type Paragraphs don't work well. This used to work fine in our old codebase. I haven't been able to troubleshoot why it's failing in LocalGov.

In any case, all tests are failing due to a broken schema in the office_hours module. Finn is working on a work-around. Once that's ready, tests should pass here as well and I will mark this change as Ready-for-review.

@danchamp @willguv

[danchamp]

Hi @Adnan-cds

Thanks for your work on this, is there anything we can do to help get it completed?

If it's ready I can test it locally and feedback.

[Adnan-cds]

Sorry Dan. Totally forgot about this one. I am trying to trigger the test run. Once that's successful, I will request you to review and test.

[danchamp]

No problem at all, I'd lost sight of it too and only picked it up reviewing Trello.

[Adnan-cds]

Tests are passing. So ready for review :)

[Adnan-cds]

Hi Dan, Let me have a quick look at both the issues. I can see that the alignment options don't work in our site even for media entities. It is possible I have missed something obvious here. I will dig into it.

[Adnan-cds]

I can confirm that the alignment works in Drupal's default Bartik theme for both Media and Page component entities. I am trying to figure out why it is failing in localgov_theme.

[Adnan-cds]

The alignment options when embedding a component show in source as data-align attributes, and persist, but aren't rendered on the front-end in any form.

Because this works in Bartik, let's open a new ticket in the localgov_theme's issue queue and resolve it there.

There's a grumble in the console about quick edit for the page component itself, with a suggestion it's theme-related:

This is due to a core bug. I have added a work around which gets rid of the error message from the Javascript console although I can't actually edit the Page component that I embedded in the WYSIWYG. I don't know if this is normal.

I was testing by embedding both a Media image and a Page component into the same WYSIWYG. I got similar error messages for the Media entity due to the core bug. I had to add the following code snippet to mute it for Media:

function a_module_preprocess_media(&$vars) {
  $media_entity = $vars['elements']['#media'];
  $media_id = $media_entity->id();
  $vars['attributes']['data-quickedit-entity-id'] = "media/{$media_id}";
}

Ready for review and testing once again.

[Adnan-cds]

it would be good to get this with an update hook or at least flag up that new config needs to be installed to use it.

Hi Andy, I didn't add any update hook to insert the Page component embed button into the WYSIWYG as this is an optional feature. Instead I have added some instructions in the README. But if you are referring to something else when you mentioned update hooks then please do elaborate and I am happy to look into that.

I fully agree that we will have to highlight this new feature in the release notes. The module versioning also merits a minor number upgrade.

This is adding entity_embed as a dependency, which has a cascading effect over the services stack

Given that entity_usage already declares a dependency on entity_embed: ~1.0, we don't really need an explicit dependency declaration for entity_embed here. I will get rid of it.

Finally, I like the 9 out of 10 Councils preferring LocalGov Drupal fake news :D

[andybroomfield]

@Adnan-cds Maybe I misundestood, as I also had to follow your instructions when doing a fresh install to get it to work. Is this config not really new? config/optional/core.entity_view_display.paragraphs_library_item.paragraphs_library_item.embedded.yml config/optional/core.entity_view_mode.paragraphs_library_item.embedded.yml config/optional/embed.button.page_component.yml config/optional/entity_browser.browser.page_components_for_wysiwyg.yml

Otherwise I'm ok if this one needs to get merged. (Entity embed isn't a deal breaker as I thought it does get used elsewhere, but was something that needed to be downloaded when switching to this branch).

[Adnan-cds]

Is this config not really new?

I get it now. These optional configs may indeed need an update hook. I will get back to you again once I have resolved that. Thanks for pointing out.

Entity embed isn't a deal breaker

Yeah, I missed that entity_usage requires entity_embed as a dev dependency. I have removed and then restored entity_embed.

[Adnan-cds]

Hi Andy, I have added an update hook that attempts to install the optional config files. Ready for review again.

[finnlewis]

This is also relevant: https://github.com/localgovdrupal/localgov/issues/320

[finnlewis]

It sounds like there is still some contention here, from discussion with @markconroy and @Adnan-cds on our tech group drop-in call... it probably needs more discussion.

We are using paragraphs in some places and body fields in others.

On the 'service page' content type we have a body field and a reusable paragraph type already. Perhaps we should be moving towards a consistent way of structuring.

We need to check with Product Group and confirm the direction here.

[finnlewis]

For now, let's not merge this, but @Adnan-cds would still love @andybroomfield to review!

[Adnan-cds]

We need to check with Product Group and confirm the direction here.

For reference, here's the original ticket for the Page component embed ticket. @finnlewis @willguv @markconroy @andybroomfield

[Adnan-cds]

Thanks Andy :)

[markconroy]

I'm trying to test this. I think we might need to update the config to have this working by default when you add the embed icons to the ckeditor toolbar.

When I added "node" and "page component" embed icons, they showed in the editor but did not do anything.

Then I enabled "Display embedded entities" and clicked save, which gave me the following error: The Node button requires <drupal-entity> among the allowed HTML tags.

Then I added <drupal-entity> to the allowed HTML tags, and clicked save, which gave me the following error:

The <drupal-entity> tag in the allowed HTML tags is missing the following attributes: data-entity-type, data-entity-uuid, data-entity-embed-display, data-entity-embed-display-settings, data-align, data-caption, data-embed-button, alt, title.

Then I added all those attributes to the drupal-entity element, and now it seems like I can start embedding things to test this.

[markconroy]

I have done some testing on this now and have come to 2 conclusions:

1. The editor experience of this is not good (we are using the default editor experience, so maybe we could improve that), and
2. I stand by my earlier criticisms of this as a feature. I think we need to spend more time figuring out the problem we are trying to solve, and then solve that in a scalable manner.

Here's a short video of me embedding a page component as the first item in a wysiwyg, and then not being able to add content after it: https://www.dropbox.com/s/jfiyjn5w8qpcxl8/Embed%20Paragraphs-%20Bad%20UX%20When%20only%20thing%20in%20editor.mp4?dl=0

Here's a short video of me trying to add a second page component in the wysiwyg when there is only one item currently in it: https://www.dropbox.com/s/gljbqr10lrlk3uq/Embed%20Paragraphs-%20Bad%20UX%20When%20adding%20or%20editing%20embeds.mp4?dl=0

Here's a short video of me creating an infinite loop (of sorts): https://www.dropbox.com/s/pao79bv2fdcltvs/Embed%20Paragraphs-%20Same%20component%20inside%20itself.mp4?dl=0

I also posted a question about this on twitter. Here are the responses so far (9 responses): https://twitter.com/markconroy/status/1441014021155663874

Ya, not sure I love that idea for all the reasons you listed.

Agree it's a bad idea. Just because you can...

It sounds terrible, but does it give a better editor experience? I’m trying to figure out how to enable my publishers rich data input without forcing them into the clunky paragraphs interface. I want Gutenberg without Gutenberg.

100% right! We had to deal with a similar situation (Drupal 7), I suggested paragraphs thinking about migration. The tech lead pushed the wysiwyg way claimimg the emergency of the task. I hate this!

I'm not a fan, for the reasons you mentioned, also because you're adding a clunky UX to a clunky UX. Non-technical users (and many technical users) will have a very hard time using that.

snt that just Gutenberg reborn?

Layout paragraphs maybe ?

I've done this in a situation where we already had a set of rich front end components powered by a paragraphs UI. Client needed a little more flexibility in certain areas with WYSIWYG so the reuse seemed to make sense.

I used this idea as a wrapper for embedding maps/location cards the other year. It needed all kinds of config and custom code to shave off the rough parts. For more general paragraphs embedding this sounds like a really tricky thing to manage or use

[tom-steel]

The needs that this fulfils are quite specific for me for displaying certain inline content. I think this would benefit from having a conversation at a product level as the points around existing and similar functions, along with the direction that Drupal core is progressing are important and we don't want to create bloat and duplication.

[willguv]

I'm glad we're having a detailed conversation and am confident we'll find a good solution in the middle of this.

@tom-steel we're planning to restart the Product Group (as a meeting of Product people rather than a big show and tell) in the next week or two. Does this work for you, or will it slow you down? Cheers

[Adnan-cds]

Thanks for trying this out Mark. The screencasts, while not fun to watch you suffer, were very useful nonetheless.

Then I added all those attributes to the drupal-entity element, and now it seems like I can start embedding things to test this.

The setup steps you have gone through are actually covered in the README. These are very similar to how you setup Media embedding in the WYSIWYG.

The editor experience of this is not good

It is almost the same as core's Media entity. Could you please try to reproduce your videos with Media entities (e.g. Media image) instead of Page components. Also, I have noticed that in the screencasts, you have totally ignored the red arrow symbol (i.e. the ⤶) that appears when you hover near the top or bottom of the embedded Page component. That's how we insert line breaks. I thought this is well known due to the widespread use of Media entities in WYSIWYG. Your editing experience would have been better had you used the line break arrow. I have never heard of any complain from our editors about Media embedding in the WYSIWYG. So I assumed Page components would be fine as well.

Dan and Andy didn't seem to have had much of an issue with the UX.

I think we need to spend more time figuring out the problem we are trying to solve, and then solve that in a scalable manner.

Tom has already explained this in a previous comment and Finn has touched it too. I will add that this feature is primarily for embedding Contact cards, Quotes, Document packs (localgov_documents) and two more paragraphs I can't remember right now! Not all content types have the Page component field and this feature could be useful for those. Even if we add the Page component field to all content types, I feel it will be still useful to add the odd paragraph in the middle of a textarea field with text flowing before and after.

Here's a short video of me creating an infinite loop (of sorts):

That's an interesting one. Once we restrict the allowed Paragraph types, of which I believe there is consensus, this will be less of an issue although you may still be able to cause this if you are sufficiently determined :)

snt that just Gutenberg reborn?

That one caught my eyes. Yes, it is, kind of. If we use this functionality to insert Paragraph after Paragraph after Paragraph inside the WYSIWYG to build a whole page then it is indeed undesirable. Our use case is to insert the odd one here and there from time to time rather than build entire pages out of it. LocalGov Drupal is a distribution for Councils and as far as I have seen, Editors working for Councils are mature enough to avoid such traps. I am more than happy to add a warning in the README in this regard.

Regarding future migration, I have done this type of migration using the Dom migration process plugin. Not the easiest, but clearly doable.

Lastly, "decoupling". Yes, that's the big one. As you said yesterday, "what if you decide to go headless in three years time?" Of all the reasons you have put forward, I think this is the only real deal breaker (as long as we restrict the Paragraph types available for embedding). I am trying to find out more about it. I will report back.

[finnlewis]

@Adnan-cds thanks for your work on this, @markconroy thanks for your thoughts and testing and thanks all for the ongoing and detailed discussion.

I think this taps into an underlying issue around consistent architectural decisions and product management, which will be best discussed with the techincal and product groups when both are rebooted. If we can hold off on this one until the product group is back in action, it would be a good one to discuss in more depth with more people to see if we can come to an overall approach that is agreeable to as many as possible.

[finnlewis]

Just checking in on this in our 'Merge Monday' meeting.

Note to self: bring Colin (Webcurl) into the discussion, as they've done something with Layout Paragraphs to answer a similar need. We're also exploring https://www.drupal.org/project/gutenberg for more intuitive editing of some pages.

Any updates from Croydon's usage of this @Adnan-cds ?

[Adnan-cds]

Any updates from Croydon's usage of this

Hi Finn, we are not using this. We were hoping to get it from LocalGov. I am trying to set apart some time to find the impact of this feature on headless sites.

[finnlewis]

Hi @Adnan-cds

We're just checking in on all open pull requests.

Is this still a live discussion?

Cheers,

Finn

[willguv]

@finnlewis Ben and other content designers are still citing the need to include an venue address (as a paragraph example) part way down a page of content

So we need to think about how to go about this

[Adnan-cds]

Hi Will, I am supposed to test this functionality within the context of a headless site. I was hoping to try this around last January but had to work on something else unfortunately. If this is needed urgently then one option would be add it as part of a custom Drupal module.

[finnlewis]

@Adnan-cds mentioned today that this is no longer a requirement at Croydon.

@markconroy raised the idea that if a single council wants a feature, it might not warrant deep discussion. If more than one council wants a thing, it is worth exploring.