Lille
Account created on 15 September 2009, almost 15 years ago
#

Merge Requests

More

Recent comments

🇫🇷France nod_ Lille

Thanks, all good.

Committed 196137e and pushed to 10.4.x. Thanks!

🇫🇷France nod_ Lille

Committed 3c4f80a and pushed to 11.0.x. Thanks!
Committed 3c4f80a and pushed to 11.0.x. Thanks!

Doesn't cherry pick to 10.4.x, need a new MR for it if we want to backport that

🇫🇷France nod_ Lille

Committed and pushed f82e35fb1f to 11.x and 62a37e84de to 11.0.x and 3ec2fa91d5 to 10.4.x and 0d664bc8b4 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Committed ae638ca and pushed to 11.x. Thanks!
Committed e1ba327 and pushed to 11.0.x. Thanks!

10.4.x is also on 42.0.0, will need a MR for it

🇫🇷France nod_ Lille

I'd like to have a function to avoid duplicating the code in two places here. The function can be local to this file, no need to add something to the global Drupal object

🇫🇷France nod_ Lille

I'm going to try and refocus the jQuery removal work and to do that I need to take a few decisions. I'm going to close a few issues that are most likely to introduce regressions and concentrate on the bigger topics.

The issues I'll be closing risk introducing problems with contrib and/or introduce too much extra code that looks similar to jQuery. jQuery can be handy, even elegant in some cases and the replacement will be harder to maintain.

I'm porting credit from closed issues to 📌 Credit for work on the reduce jQuery issues Active because some of those issues have significant work behind them

🇫🇷France nod_ Lille

First of all thank you all for the hard work on this one, I've had to push a number of patches like this and I know how hard it is to keep up.

I'm going to try and refocus the jQuery removal work and to do that I need to take a few decisions. I'm going to close this issue for a few reasons:

  • This MR is big, it impact a very big number of subsystems and make the code more brittle. jQuery is good at dealing with undefined elements, empty sets and so on, the DOM isn't. We already had regressions from a previous patch with undefined elements
  • The MR is too big to review and make sure there are no regressions (even with the tests we already have), it would create unstability that we don't have to endure
  • Sometimes the jQuery code is simply more readable, I do not think this change is a net positive:
          $(once('filter-guidelines', '.js-filter-guidelines', context))
            .find(':header')
            .hide()
            .closest('.js-filter-wrapper')
            .find('select.js-filter-list')
            .on('change.filterGuidelines', updateFilterGuidelines)
            // Need to trigger the namespaced event to avoid triggering formUpdated
            // when initializing the select.
            .trigger('change.filterGuidelines');
          // Define the closest function for finding the closest ancestor with a given selector
          function closest(element, selector) {
            while (element && !element.matches(selector)) {
              element = element.parentNode;
            }
            return element;
          }
    
          const contextElement = document.querySelector('.js-filter-guidelines');
          const closestWrapper = closest(contextElement, '.js-filter-wrapper');
          if (closestWrapper) {
            const $context = $(contextElement);
            const selectElement = $(closestWrapper).find('select.js-filter-list');
            $context.find(':header').hide();
            selectElement
              .on('change.filterGuidelines', updateFilterGuidelines)
              // Need to trigger the namespaced event to avoid triggering formUpdated
              // when initializing the select.
              .trigger('change.filterGuidelines');
          }
    

    There is one line to change and we have to make many, many other changes for this to work

I ported the credits to 📌 Credit for work on the reduce jQuery issues Active , which I will mark as fixed as soon as I go through all the others impacted jQuery issues.

🇫🇷France nod_ Lille

I like it, we do need to add the deprecation of the jQuery event here. And introduce a BC layer for events so that calling $dialog.trigger('event'); triggers a deprecation and generate the custom DOM event.

🇫🇷France nod_ Lille

Committed and pushed e8cafee5ae to 11.x and 58f4b99d0f to 11.0.x and 1009866a33 to 10.4.x and a3075fda43 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

I reviewed the issue and all the different solutions change the design of the comment section one way or another. Changing the design of the comment section is not in the scope of this issue.

The issue is that inside the LB UI the icon is outside the LB section. This is by design that the picture is out of the normal flow of the page, it makes sense that it is also out of the flow of the LB interface. If we make it fit inside the LB the preview would not be accurate anymore.

Moving the comment element to the right to avoid this, but it breaks the intended design of the page. If the overlap is problematic it's always possible to disable the content preview with the checkbox on top of LB UI. Because we do not want to change the design of the comment and there is a solution to avoid this I'm closing this as works as designed.

Thanks to everyone who worked on this over the years. I wish we caught this one earlier to avoid spending so much time on this.

🇫🇷France nod_ Lille

Committed and pushed c0e94e3446 to 11.x and 87b8c59a49 to 11.0.x and 9e73b2b33f to 10.4.x and 2328738ae7 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Committed and pushed 27fcad90d9 to 11.x and fd1bcbe5aa to 10.4.x. Thanks!

🇫🇷France nod_ Lille

Committed and pushed fe4afc9a13 to 11.x and 696b56bec1 to 11.0.x and be2804a4c2 to 10.4.x and 202efea7bd to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Committed 4c3ce18 and pushed to 11.x. Thanks!

🇫🇷France nod_ Lille

Committed and pushed 3dc298586a to 11.x and 27194eb024 to 11.0.x and ee43e75bb6 to 10.4.x and 243fc5ca9e to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Committed and pushed 17aee79159 to 11.x and ae41c62daf to 11.0.x and 8912d59338 to 10.4.x and da2452190d to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Committed f1769fa and pushed to 11.x. Thanks!

🇫🇷France nod_ Lille

That makes sense to me, do we have a use case for this today? if it helps existing tests we should convert at least one to showcase this.

🇫🇷France nod_ Lille

I'd like to get this in 10.3 so we need an update function to make sure cache gets cleared (per catch)

🇫🇷France nod_ Lille

I went through the same though process as you @catch and got the same "umm I don't know but I don't have a clearly better solution" conclusion. I should have commented, it could have saved you a bit of time.

I'll leave the commit to someone else too, this one is a bit out of my scope.

🇫🇷France nod_ Lille

Thanks, just a dependency missing for safe triangle lib.

Also if your site uses navigation today it breaks it until you clear the cache. Should we have an empty update function to make sure it's done?

🇫🇷France nod_ Lille

Committed and pushed 90f8dd194f to 11.x and 351c0873ee to 11.0.x and f9e6633c9a to 10.4.x and 42a97ff4f7 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Committed and pushed 52a6386a01 to 11.x and 8db0e74031 to 11.0.x and 66aed93e70 to 10.4.x and 72e4fe38f1 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Found more bugs. At this point I'm not convinced the .each() method is worth replacing given the size of this MR and all the things that it could break.

I pinged @bnjmnm so that he could give his opinion on it

🇫🇷France nod_ Lille

Committed and pushed c549a82cbc to 11.x and 16d07a5ff5 to 11.0.x and 48ed4f5e05 to 10.4.x and 4e93352ccb to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Looking good, thanks

🇫🇷France nod_ Lille

new beta for jquery 4 got out, need a new issue to update in 11.x https://blog.jquery.com/2024/07/17/second-beta-of-jquery-4-0-0/

🇫🇷France nod_ Lille

Committed and pushed c1708b3265 to 11.x and 9c482363bb to 11.0.x and 1dcc53dd09 to 10.4.x and eacdda9692 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Would make sense to make splitbutton a webcomponent too and not do it for this one. If we can't make all the dropbutton into splitbuttons at once it still makes sense to keep this one around.

This one was a test to check testability in our stack and the gotchas of using Drupal.theme()

🇫🇷France nod_ Lille

Thanks, I realized that 6 months ago I found the CSS only solution but browser support was not there yet, now it's been a while and all the supported browsers caught up.

🇫🇷France nod_ Lille

Committed c1e7cc2 and pushed to 11.x. Thanks!

🇫🇷France nod_ Lille

Thanks for sticking with it, found a way to simplify a number of selectors thanks to :has()

🇫🇷France nod_ Lille

CR was a bit too short, added a bit of context :) Since this is sessionStorage there is no need to get rid of the old variable, it will be deleted when the tab closes.

I know it's now in the toolbar module but it's something that is used in gin, will be used by the navigation module eventually, and the toolbar module will be deprecated/removed at some point. So not sure it's necessary to namespace it in the toolbar. We'd need to do another deprecation later and we don't need to go through that I think.

Can we do Drupal.escapeAdminPath instead?

🇫🇷France nod_ Lille

Committed and pushed 86138d4c11 to 11.x and 98f8509200 to 11.0.x and 1f4aa5cf19 to 10.4.x and 873b1e7b07 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

ok so with selenium, using webcomponents needs to happen through some JS, not with the driver methods, then it works.

Not ideal but at least it's green.

Had to mess with file weight for the JS so that the theme function is used for claro, that might cause some issues in contrib. Would have been ideal to have the before/after thing for scripts to replace weights.

🇫🇷France nod_ Lille

Committed e700aa8 and pushed to 11.x. Thanks!

🇫🇷France nod_ Lille

I would really like to avoid having yet another filtering script (we already have at least 3 that do something similar in a different way).

Can we do something based on Create Javascript library for searching rendered lists on the client. Needs work instead?

🇫🇷France nod_ Lille

Committed 6aa443c and pushed to 11.x. Thanks!

🇫🇷France nod_ Lille

Thanks for the update, since it's pretty minor I'd rather not reopen this one, especially since the tracker module is out of 11.x

🇫🇷France nod_ Lille

Committed and pushed 01b29f3359 to 11.x and 8db0f86221 to 11.0.x and b61e89595c to 10.4.x and 24c1497aba to 10.3.x. Thanks!

🇫🇷France nod_ Lille

the tests in machineNameTransliterationTest.js are not correct, it doesn't test how things are used in Drupal from the PHP

🇫🇷France nod_ Lille

I think the problem is elsewhere. We're passing a regex to slugify for the allowedChars option where it expects a list of characters (so, not a regex). It's fine to do that because the replace pattern is used before sending the string to slugify.

Tried a different approach, let's see what testbot says.

🇫🇷France nod_ Lille

I think we need a dedicated 10.x branch since that doesn't cherry pick cleanly

🇫🇷France nod_ Lille

Perfect, thanks!

Committed and pushed c3383f44d5 to 11.x and 9a8588df4b to 11.0.x and 7beacd4a87 to 10.4.x and 9f14510102 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Fix is not correct in related issue. closed this one to continue work on the other one

🇫🇷France nod_ Lille

In 📌 Claro should use libraries-extend for views_ui.css Fixed we removed the views-ui.css file from the global stying but mistakenly added the file to the views/views.module when it should have been views_ui/admin.styling.

🇫🇷France nod_ Lille

Thanks for keeping up with this massive MR :)

Few more comments and we should be good to go.

🇫🇷France nod_ Lille

Committed and pushed b06bf11bfb to 11.x and 573404dd22 to 11.0.x and 84cebb07ea to 10.4.x and 7608e93ebc to 10.3.x. Thanks!

🇫🇷France nod_ Lille

we don't need to replace the customevent dispatch with jquery trigger, it's on purpose that it's a dom event now.

🇫🇷France nod_ Lille

Committed and pushed bff71e54fc to 11.x and 653c57335b to 11.0.x and 03f71c7351 to 10.4.x and dfbffd2a19 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

we fixed a few issues that made HTML invalid over the years (duplicate ids, closing tags, that kind of things). At least we don't add known invalid html on purpose.

🇫🇷France nod_ Lille

It compress very well, better focus on something else if page weight is an issue

🇫🇷France nod_ Lille

I checked the htmx code and they're querying the DOM for both hx-* and data-hx-* attributes at the same time : https://github.com/bigskysoftware/htmx/blob/master/src/htmx.js#L337 so there is no performance impact here, it's the DOM doing the work in either case

🇫🇷France nod_ Lille

Committed and pushed b97be27a89 to 11.x and 68a22a5cc0 to 11.0.x and 6e1e0c29d0 to 10.4.x and d0592e79ef to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Thanks for the workaound but let's drop the htaccess changes, since we're keeping the old images for now it won't be an issue. Removing in the next major is enough.

🇫🇷France nod_ Lille
    Remaining self deprecation notices (5)
    
      5x: Javascript Deprecation: dialogClass is deprecated in drupal:10.4.x
    and will be removed from drupal:12.0.0.
        4x in MediaLibraryTest::testAllowedMediaTypes from
    Drupal\Tests\ckeditor5\FunctionalJavascript
        1x in MediaLibraryTest::testAlt from
    Drupal\Tests\ckeditor5\FunctionalJavascript
🇫🇷France nod_ Lille

Had to revert on 10.4.x as there are test failures due to the code still using dialogClass somewhere.

🇫🇷France nod_ Lille

not committing to 10.3.x because of the id removal, might backport once follow-up is in.

Committed and pushed 9e83195c5d to 11.x and 07d570cfc7 to 11.0.x and 74cd44af63 to 10.4.x. Thanks!

🇫🇷France nod_ Lille

Committed and pushed 1b5c7db4ee to 11.x and 7be520e6e5 to 11.0.x. Thanks!

🇫🇷France nod_ Lille

No need for follow-up, we override the _createWrapper method where this is set initially. Ported the relevant line in core and updated the MR

🇫🇷France nod_ Lille

Added this issue to the CR: https://www.drupal.org/node/3440844

Committed and pushed eeaf9c829e to 11.x and 71230ba40a to 11.0.x and ec9ed86905 to 10.4.x. Thanks!

🇫🇷France nod_ Lille

It's great that it natively supports data-hx- attributes. The great thing is that it doesn't prevent people from using hx- at the same time. It's not everyday there is no downsides :)

We should use the data-hx- form in Drupal since we're following the standards.

🇫🇷France nod_ Lille

Can you open a follow-up for the aria modal issue?

🇫🇷France nod_ Lille

you need to keep

        wrapper.removeAttribute('data-drupal-messages-fallback');
        wrapper.setAttribute('data-drupal-messages', '');

outside the if (!wrapper)

🇫🇷France nod_ Lille

Reviewed by a subsystem maintainer

Committed and pushed 1a487f3b7c to 11.x and 4074145ef7 to 11.0.x and 5601dc41ea to 10.4.x and acbd64f9b0 to 10.3.x. Thanks!

🇫🇷France nod_ Lille

Couple of things to simplify then it's good to go, thanks!

Production build 0.69.0 2024