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
before

After
after

API changes

NA

Data model changes

NA

Release notes snippet

Issue fork drupal-3100083

Command icon 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:

Comments

zrpnr created an issue. See original summary.

zrpnr’s picture

StatusFileSize
new4.57 KB

This patch adds a js file for overriding the theme function, and uses the markup from the twig file.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

shriaas’s picture

Status: Active » Needs review

Patch #2 needs review

zrpnr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Looks like it needs a reroll for 9.3.x now

adityasingh’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new4.62 KB

Reroll the patch for 9.3.x.

mitthukumawat’s picture

StatusFileSize
new85.08 KB
new57.92 KB
new139.77 KB

@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.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs screenshots

This 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

markconroy’s picture

This looks really good. Just a few things to consider before we give it a final test/merge.

  • In Drupal 10, we don't need to have the es6 code transpiled for ie11 any more, so that can be removed.
  • Can we try remove the weight from the CSS file in the library definition. I don't think that should be needed.
_utsavsharma’s picture

StatusFileSize
new1.62 KB
new3.11 KB

Rerolled patch for 10.1.x.

ankithashetty’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new3.23 KB
new2.68 KB
new3.36 KB

Tried 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.js into .js file from the D9 patch and remove the .es6.js.
#14.2 - not sure whats the ask.

Thanks!

smustgrave’s picture

Status: Needs review » Needs work

Thank you for working on this @ankithashetty

Will still need screenshots for this though.

markconroy’s picture

Hi @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.

Zsuffa Dávid’s picture

Status: Needs work » Needs review
Issue tags: -Needs screenshots
StatusFileSize
new22.96 KB
new23.68 KB
new35.52 KB
new28.81 KB
new2.9 KB
new1.48 KB

I made a slight modification of patch #16.
Message.css was added back to the library, adding an id to the h2 for the aria-labelledby attribute.
I also added a wrapper span around the message text to match the twig markup.
Before/after screenshots are attached.

smustgrave’s picture

Issue summary: View changes

Thanks 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.

Zsuffa Dávid’s picture

StatusFileSize
new2.82 KB
new881 bytes

I 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-labelledby attribute since the h2 is visually-hidden but it contains the text needed for screen readers.
Added new patch.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Change looks good and since this is a demo profile doubt it will need any kind of change record

Good job!

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/profiles/demo_umami/themes/umami/umami.info.yml
@@ -7,7 +7,6 @@ libraries:
-  - umami/messages

If we remove the umami/messages library from the global libraries, it isn't guaranteed anymore that the message styles are loaded when messages are rendered. We probably should add the messages.css styles to the global library since it should be always loaded.

gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new3.2 KB
new680 bytes

Added messages.css to global library.

I have added patch and interdiff for same. Please review

smustgrave’s picture

Status: Needs review » Needs work

If we add to global don't think we need

+  core/drupal.message:

Anymore right?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Category: Feature request » Task
Priority: Normal » Major
Related issues: +#3396318: AJAX MessageCommand markup and styling differs from Theme default

finnsky made their first commit to this issue’s fork.

finnsky’s picture

Found 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.

finnsky’s picture

One 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

finnsky’s picture

Status: Needs work » Needs review

Messages fixed + optimized

finnsky’s picture

I 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!

smustgrave’s picture

The fact this seems to hit performance is it still a task to do?

finnsky’s picture

This hits performance mostly because I've added lost drupal/message dependency

finnsky’s picture

Issue summary: View changes
markconroy’s picture

@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.

finnsky’s picture

@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

smustgrave changed the visibility of the branch 3100083-web-components to hidden.

smustgrave’s picture

Rebasing 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

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Ok 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.

nod_’s picture

small question in the MR

finnsky’s picture

  • nod_ committed b89363b1 on 10.3.x
    Issue #3100083 by finnsky, Zsuffa Dávid, ankithashetty, _utsavsharma,...

  • nod_ committed ae988430 on 10.4.x
    Issue #3100083 by finnsky, Zsuffa Dávid, ankithashetty, _utsavsharma,...

  • nod_ committed 138d5889 on 11.0.x
    Issue #3100083 by finnsky, Zsuffa Dávid, ankithashetty, _utsavsharma,...

  • nod_ committed 0e07179f on 11.x
    Issue #3100083 by finnsky, Zsuffa Dávid, ankithashetty, _utsavsharma,...
nod_’s picture

Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -messages, -Needs framework manager review, -Javascript Modernization Initiative, -web-components +JavaScript, +performance budget change

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!

  • nod_ committed 6f000e33 on 10.3.x
    Issue #3100083 by finnsky, Zsuffa Dávid, ankithashetty, _utsavsharma,...

  • nod_ committed 6e5e886d on 10.4.x
    Issue #3100083 by finnsky, Zsuffa Dávid, ankithashetty, _utsavsharma,...

Status: Fixed » Closed (fixed)

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