Replace js message with webcomponents

Only tagged for that as there was some discussion to still be had it seemed from the original issue.

Recreated MR ro review this as Webcomponent only, without previous context.

I'm happy with this so. Let's RTBC.

---
Thanks to The Confident for sponsoring my time to work on this.

Have either of you looked at 🐛 AJAX MessageCommand markup and styling differs from Theme default Active which is trying to do this generically?

@catch.

Pushing it to core will be long and complicated. In same time Umami is experimental. Let's do it here and link it in that issue?

I agree with @finnsky. Let's do it here for Umami.

We can see how it fares and remove it when the generic version is ready, while also using our version of it as a testing ground.

Moreover, it seems to me that here we do it much more elegantly than in the attached task. Using progressive but at the same time already well-documented technology.
https://developer.mozilla.org/en-US/docs/Web/API/Web_components

So after testing here, in my opinion, we should move this to the core in linked task

Web components, +1 I don't think there is any concerns with using that in core beside the naming convention. For Shadow Dom (and especially declarative shadow dom) it's less clear. There are two topics

1. style isolation and how much harder will it be to change from contrib/projects?
2. Slots, the concept is great but it does come with potential issues

To elaborate on #2, when you have

<template id="umami-messages-template">
  <h2 class="heading">
    <slot name="title"></slot>
  </h2>
</template>

<!-- then -->

<umami-messages-component>
 <span slot="title">Something</span>
</umami-messages-component>
 

Accessibility picks up the h2, but it doesn't seem that you can access it from the JS. Just need to make sure we're all fine with that.

it's late so wrapping up here for today

To allow styling specific sections in shadow dom, we can always use ::part() https://developer.mozilla.org/en-US/docs/Web/CSS/::part

I did not understand what was the potential issue with Slots / h2 / accessibility. Can you please try explain it again?

will come back with more later today/tomorrow but there are still feedbak in the MR to address

Sending to review. Plus a11y tag.

On my side axe report is fine with message on page.

I've added fix but i don't really like it. I would better avoid changes on core level in this MR. We can do this fix later. When we get more info

Reverted last commit

The code itself looks good. It's nicer to use slots than mess with theme functions. I'm still on the fence adding shadow dom on a custom core element.

Given the fact that it's an experimental theme that we'll retire at some point, why not. I would like to have a disclaimer somewhere in the code (a twig comment above the <template> element maybe?) saying that this is a test, not a guideline on how to use template/shadow dom in Drupal. We haven't talked through all the topics (how/where do we document the slots, js required, style duplication, different styling methods, etc.) hence the disclaimer. Most of the topics would need to have an answer or general agreement before we use this in non experimental parts of core. in the spirit of getting things going I'm ok with committing this

NW for the disclaimer comment

Thank you for review.

I added a disclaimer. Not sure what form it should take. But I tried to be formal and bureaucratic :)

Please take a look!

+1 to Pam’s content suggestion.

nod_ → credited pameeela → .

thanks @quietone and @pameeela!

Disclaimer looks good to me too. If we standardize this in 🐛 AJAX MessageCommand markup and styling differs from Theme default Active or a similar issue we can update the Umami one to be inline and remove the disclaimer again (or still keep it there if we want).

finnsky → changed the visibility of the branch 3476471-replace-js-and to hidden.

finnsky → changed the visibility of the branch 3476471-replace-js-and to active.

Fixed up the IS a little bit and resolved the threads

Applying the patch and doing a fresh install.

MR applied but did get

test.patch:169: trailing whitespace.
Umami uses a web component for rendering messages. The component is
test.patch:170: trailing whitespace.
““.
test.patch:171: trailing whitespace.
test.patch:172: trailing whitespace.
This implementation is a trial only and should not be used on production
warning: 4 lines add whitespace errors.

So think those need to be fixed. Fine to self RTBC after

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Done!

@smustgrave I think I may have jumped in. I saw #34 'done' so checked and saw one failing test. Wasnt sure it was a random failure but since just experienced a single random failure in another issue, re-ran and it passed. So ran 'test-only' test. It passes. Since there is test coverage in the MR, that should not happen? So 'needs work' to make the test effective?

Perhaps I should have commented to @finnsky to carry out these steps? But from experience explaining the test-only test, why to run it, how to run it etc. seems so much harder than just running it and then explaining what the result means..

nod_ → closed merge request !9819

Committed 70b2d5e and pushed to 11.x. Thanks!

Great news! Thank you!

Automatically closed - issue fixed for 2 weeks with no activity.