Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
"Once" is not yet in core. See #2402103: Add once.js to core
This task comes from a code review in #73 in #3111409: Add new Olivero frontend theme to Drupal 9.1 core as beta
Olivero will be able to use the vanilla once script once it's in core, for now it's ok to leave it like that. It doesn't make sense to require jquery for such a small thing. the once feature is handled Drupal 6 style. good enough for now.
Comment | File | Size | Author |
---|---|---|---|
#22 | 3173900-17-reroll.patch | 7.62 KB | Spokje |
#17 | 3173900-17-reroll.patch | 7.62 KB | mherchel |
#11 | interdiff-8-11.txt | 3.18 KB | mherchel |
#11 | 3173900-11.patch | 7.6 KB | mherchel |
#8 | 3173900-8.patch | 7.61 KB | mherchel |
Comments
Comment #2
mherchelComment #3
nod_To do right after this one lands
Comment #4
nod_once js is in core now :)
Comment #5
nod_Comment #6
mherchelFirst pass at this is attached. Seems super easy to use!
Comment #7
nod_you can simplify with
once('olivero-messages', '.messages', context)
:)same here:
once('olivero-navigation', `#${navWrapperId}`, context)
same
tabs.forEach(init)
is sufficientComment #8
mherchelThanks for the quick review. Here's the changes requested.
Comment #9
nod_Here you can simplify as well since the messages const is not used later:
This one doesn't work right? Once always returns an array and because later you use navWrapper as a DOM element you can do either:
Here as well the tabs const is not used later so it can be a little shorter:
Comment #10
droplet CreditAttribution: droplet commentedIt seems like a bug to me
In what case the `Drupal.theme.message` is called but no `closeMessage`?
https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/themes/olive...
If we have a special custom function without the `closeMessage`, should we inject it?
Comment #11
mherchelChanges from review in #9 attached
Comment #12
nod_#10 when a message is displayed by the backend (when saving a node for example) the message is on the page but Drupal.theme.message was not called to display it so we need the utility function (the naming is confusing though). Or did I miss something else?
Patch is good for me.
Comment #13
droplet CreditAttribution: droplet commented#10,
OK. What blocks us to do it on client-side only? (and must be client-side only?)
https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/themes/olive...
OK. So my best guess is the `click handler` and we don't want to repeat the code again on serverside.
EDIT:
and if I read the code correctly, #11 won't prevent an extra call on `closeMessage` (if we called it from clientside)
Comment #14
mglamanUber nits
EDIT: I didn't realize this was the generated code. Review item is moot.
The diff doesn't give much for clarity, but is there a reason to have concat here anymore, if it's just `type`? Would
`messages-list__item messages messages--${type}` be more appropriate?
Comment #15
nod_for 2. the patch is using that in the es6.js file, it's the compiled code that uses concat, so it's all good on that front :)
Comment #16
mglamand'oh! Thanks, @nod_. I tried to make sure I avoided reviewing the compiled code.
Comment #17
mherchelRe-roll attached.
@mglaman you mention core/themes/olivero/js/messages.es6.js in #14, but I don't see anything wrong. Is there an issue there? If not, does this patch look good to you?
Comment #18
mglamanYeah, sorry. My review is moot. The compiled JS doesn't have a new line and my second thing was also compiled CSS. Lookin' great to me!
Comment #20
benjifisherPlease do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.
Comment #21
kuldeep_mehra27 CreditAttribution: kuldeep_mehra27 at Valuebound for Valuebound commentedComment #22
SpokjeNot quite sure what's going on with patch #21.
As @benjifisher explained in #20 the RTBC patch #17 was failing due to an error in HEAD.
#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest was committed. Re-uploaded the original patch #17 so it will be retested on a 2 daily base, ordered retest and put this issue back to RTBC per #18.
@kuldeep_mehra27 Please provide an interdiff with updated patches.
Comment #24
lauriiiCommitted 4814a98 and pushed to 9.2.x. Thanks!