Problem/Motivation
Messages can be added with js using Drupal.Message, see: https://www.drupal.org/node/2930536
but the markup output does not match Umami's in
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/profiles/demo_...
Claro overrides the theme function: Drupal.theme.message
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/themes/claro/j...
If Umami had a similar override then js messages would look more like the twig messages.
Steps to reproduce
Proposed resolution
Add a Drupal.theme.message function for Umami to override core/misc/message.js and output markup that matches the twig template.
1. 3100083-add-js-message is actual fix branch.
2. 3100083-web-components - an experimental branch with web components. Both message methods (Drupal render and javascript theme function) use the same template.
Remaining tasks
Review
User interface changes
Before

After

API changes
NA
Data model changes
NA
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | interdiff-21_24.txt | 680 bytes | gauravvvv |
| #24 | 3100083-24.patch | 3.2 KB | gauravvvv |
| #21 | interdiff-3097503-19-21.txt | 881 bytes | Zsuffa Dávid |
| #21 | 3100083-21.patch | 2.82 KB | Zsuffa Dávid |
| #19 | interdiff-16-19.txt | 1.48 KB | Zsuffa Dávid |
Issue fork drupal-3100083
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3100083-web-components
changes, plain diff MR !8857
- 3100083-add-js-message
changes, plain diff MR !8856
Comments
Comment #2
zrpnrThis patch adds a js file for overriding the theme function, and uses the markup from the twig file.
Comment #6
shriaasPatch #2 needs review
Comment #7
zrpnrLooks like it needs a reroll for 9.3.x now
Comment #8
adityasingh commentedReroll the patch for
9.3.x.Comment #9
mitthukumawat commented@adityasingh
I have applied this patch in drupal 9.3.x-dev and reviewed the status messages. The messages are appearing as per patch markup
messageWrapper.innerHTML = "\n <div class=\"messages__content container\">\n <h2 class=\"visually-hidden\">\n ".concat(messagesTypes[type], "\n </h2>\n ").concat(text, "\n </div>\n ");It has added the status messages as per their types.
Adding screenshots for reference.
Comment #13
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Tagging for a reroll for 10.1.x
Tagging for screenshots of the before and after
Comment #14
markconroy commentedThis looks really good. Just a few things to consider before we give it a final test/merge.
Comment #15
_utsavsharma commentedRerolled patch for 10.1.x.
Comment #16
ankithashettyTried to fix test errors in #15.
As mentioned in #14.1, Drupal 10 doesn't need to have the es6 code transpiled, so we can just copy the contents from
.es6.jsinto.jsfile from the D9 patch and remove the.es6.js.#14.2 - not sure whats the ask.
Thanks!
Comment #17
smustgrave commentedThank you for working on this @ankithashetty
Will still need screenshots for this though.
Comment #18
markconroy commentedHi @ankithashetty
Thanks for this work; it's looking really good.
For #14.2, that was my mistake. I thought we were introducing a 'weight' to the CSS file, but looks like we were removing it.
However, we now have a 'messages' library with no messages.css file in it. It seems to have been moved to the general umami global library. Can we add that CSS file back to the messages library instead please, so we keep the CSS and the JS for this component in the same library?
Also, as @smustgrave says, we'll need to get some before/after screenshots to verify this. Then we can move it to RTBC.
Comment #19
Zsuffa Dávid commentedI made a slight modification of patch #16.
Message.css was added back to the library, adding an
idto theh2for thearia-labelledbyattribute.I also added a wrapper
spanaround the message text to match the twig markup.Before/after screenshots are attached.
Comment #20
smustgrave commentedThanks just FYI for next time went screenshots are tagged they need to be added to the issue summary.
For the ID is there a possibility there could be 2 of the same on the page? That could introduce an accessibility issue.
Comment #21
Zsuffa Dávid commentedI based this modification on claros's messages.js, but you are right, if two
ids are the same it can cause accessibility issues.So we can remove the
aria-labelledbyattribute since the h2 isvisually-hiddenbut it contains the text needed for screen readers.Added new patch.
Comment #22
smustgrave commentedThanks! Change looks good and since this is a demo profile doubt it will need any kind of change record
Good job!
Comment #23
lauriiiIf we remove the
umami/messageslibrary from the global libraries, it isn't guaranteed anymore that the message styles are loaded when messages are rendered. We probably should add themessages.cssstyles to the global library since it should be always loaded.Comment #24
gauravvvv commentedAdded
messages.csstoglobal library.I have added patch and interdiff for same. Please review
Comment #25
smustgrave commentedIf we add to global don't think we need
Anymore right?
Comment #27
catchComment #29
finnsky commentedFound one more bug. When Login with wrong password.
https://gyazo.com/6e5b6146b61a8d1d65249614dcb67f78
It not happends on
1. Olivero because it attaches library in template
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/oliver...
2. Claro because it has direct dependency
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/claro/...
Gonna add dependency aswell.
Comment #31
finnsky commentedOne more bug.
Seems messages_list wrapper missed. So new messages added directly inside existing message
https://gyazo.com/dff6c11b71c3fbe18661379e45e01b8f
Same bug on layout builder page
https://gyazo.com/702afeb6ed68fd2a20622f555ac8c450
Comment #32
finnsky commentedMessages fixed + optimized
Comment #34
finnsky commentedI also added an experimental branch with web components. I believe that since Umami is an experimental theme, we can easily try this technology here. Browser support suits us.
The benefits we are achieving now:
- Both message methods (Drupal render and javascript theme function) use the same template.
- Drupal has an example of using web components in its core and keeps up with the times
Please review!
Comment #35
smustgrave commentedThe fact this seems to hit performance is it still a task to do?
Comment #36
finnsky commentedThis hits performance mostly because I've added lost drupal/message dependency
Comment #37
finnsky commentedComment #38
markconroy commented@finnsky this looks very interesting.
I wonder should we do it in 2 stages: get the original JS template MR merged, and then follow-up with a proposal to use Web Components?
---
Thanks to The Confident for sponsoring my time to work on this.
Comment #39
finnsky commented@markconroy yes, 2 stages ok.
Second MR here it is exact fix here.
Let's move Webcomponents into new issue and let community to discuss
Comment #41
smustgrave commentedRebasing 8856 as it was 200+ commits back.
Hiding the other MR and opened #3476471: Replace js and twig messages with webcomponent Umami-message for more discussion
Comment #42
smustgrave commentedOk applied the MR after the rebase and no issues/failures came up.
I created a few pages to get that status message "Article has been created" and it looks as it did before.
Don't see anything wrong in the MR, this still needs framework manager sign off but will put into that bucket.
Comment #43
nod_small question in the MR
Comment #44
finnsky commented@nod
this is answer. thank you for review.
https://www.drupal.org/project/drupal/issues/3100083#comment-15692187
Comment #50
nod_thx.
Committed and pushed 0e07179f7bd to 11.x and 138d5889080 to 11.0.x and ae988430640 to 10.4.x and b89363b18d4 to 10.3.x. Thanks!