Lille
Account created on 15 September 2009, over 14 years ago
#

Merge Requests

More

Recent comments

πŸ‡«πŸ‡·France nod_ Lille

credit to longwave and quietone for helping with testing decision

πŸ‡«πŸ‡·France nod_ Lille

hiding patch to make sure folks look at the MR

πŸ‡«πŸ‡·France nod_ Lille

Thanks for the fix and thanks for including a test, in this case the fix doesn't warrant a test and I removed it on commit.

Committed and pushed 8f25e5c227 to 11.x and 0c542167d6 to 11.0.x and 0dd26ceb53 to 10.4.x and 4f0a7ce2c8 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed cdc8135c56 to 11.x and 87ae449bf4 to 11.0.x and 8ac8832201 to 10.4.x and 2fffa8f31d to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed 275cf72083 to 11.x and 6178774238 to 11.0.x and c80972a291 to 10.4.x and cc96f97756 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Since this only impacting umami I'm backporting back to 10.3.x

Committed and pushed 587350c208 to 11.x and f607b858d6 to 11.0.x and 57a0cd753d to 10.4.x and b6026c1215 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed 5686b0c and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed bc18f36525 to 11.x and f0fefd6153 to 11.0.x and 7ca75f443f to 10.4.x and c86c46871a to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed 1fd796042d to 11.x and a7a9469cdc to 11.0.x and a989b1dfc9 to 10.4.x and 8144350adb to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Still relevant today. "DOM is slow and has a lot of overhead." this is not true anymore, the DOM is fine.

πŸ‡«πŸ‡·France nod_ Lille

hiding patch to prevent the bot from messing with the issue

πŸ‡«πŸ‡·France nod_ Lille

why the class name change? looks like it wouldn't cause a problem to use the sticky-enabled class to apply the css instead of creating a new one.

πŸ‡«πŸ‡·France nod_ Lille

The issue comes from a slack discussion which was in essence. Let's put UI suite in starshot to let users try the "site build able components" parts of XP builder now rather than later. So a bit redundant to put that here imo, but it can't hurt

πŸ‡«πŸ‡·France nod_ Lille

Since we use jQuery UI we have to use some js for positioning. If we had native dialogs it would be easier to go with css-onlu for positioning.

πŸ‡«πŸ‡·France nod_ Lille

haven't tested it yet but code looks good, I'm +1 on using floating ui for that

πŸ‡«πŸ‡·France nod_ Lille

Thanks! That's an interesting project. Haven't had time to dig into the code but happy to see alternative takes being explored.

I'm personally for htmx because the philosophy is aligned with Drupal and what we try to do. And I like that a lot of things live in the markup instead of hidden away In a js object somewhere.

πŸ‡«πŸ‡·France nod_ Lille

Agreed with #5 about scope. For the poc a new #htmx key sounds fine but we'll need to make existing code work as-is as much as possible down the line

πŸ‡«πŸ‡·France nod_ Lille

can we add a comment as to why we need to have the height changed in the CSS? I'm sure i'll wonder why it's there in a few months.

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed 7ac4979e2e to 11.x and ccc368e015 to 11.0.x and 26aafb10e3 to 10.4.x and cc27b4df99 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed 799802965e to 11.x and 53c88323af to 11.0.x and 5d9fa4ef0d to 10.4.x and 4db4f4b4b4 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Haven't tested it yet but I like this much better, thanks

πŸ‡«πŸ‡·France nod_ Lille

umm maybe that needs to be a follow-up issue

πŸ‡«πŸ‡·France nod_ Lille

I'm not sure this is the right fix. I can still get the menu icon offset with the patch on the desktop resolution with the menu collapsed. it does fix the mobile use case but I think we need to fix it once for all the use cases.

I think the root of the issue is this rule:

body.is-fixed .container {
  width: calc(100% - var(--drupal-displace-offset-left, 0px) - var(--drupal-displace-offset-right, 0px));
}

This doesn't seem to work like expected. this would make sense for absolute/fixed positioned elements, but .containers are not, from what I can see they're managed with flexbox, so we don't need to account for anything special.

There is another issue with displace, there is a 9px offset on top, when there should be none.

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed c0beeeea5f to 11.x and 6974f8b7f0 to 11.0.x and 04fad10830 to 10.4.x and 2237b2eed8 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed 924d567aac to 11.x and 1e0296d353 to 11.0.x and 4c8f3371e4 to 10.4.x and 57e4625a18 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

This needs a default value for the css variable. If displace is not used on the page (no toolbar or navigation module) the skip link takes the whole width instead of the body width

πŸ‡«πŸ‡·France nod_ Lille

Fixed css lint error on commit

Committed and pushed 5e0d1efa0a to 11.x and 0b36d01ad6 to 11.0.x and c1f7726488 to 10.4.x and 12b546ca74 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed f5cc65456a to 11.x and cc355625ad to 11.0.x and 05b6144bd0 to 10.4.x and 270f95ac48 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

The existing tabbingmanager feature has tests, this one should too

πŸ‡«πŸ‡·France nod_ Lille

Thanks for the patch, I'd rather keep and try to solve the issues raised in the MR. Setting back status from #14.

πŸ‡«πŸ‡·France nod_ Lille

This fixes it in claro, need to fix umami, it does the same center positioning think as before. Olivero is fine (design is different but it's top-left aligned already)

πŸ‡«πŸ‡·France nod_ Lille

RTBC looks fine, Can I get the changes in a MR please? Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed 91e3259 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed and pushed e8298e81ba to 11.x and 22b06e0042 to 11.0.x and 8e344072b3 to 10.4.x and 271a556170 to 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

we can see in the screenshot from #23 that the padding is different in the dialog. Is there no way to avoid adding specificity to the .button--danger selector? BEM is supposed to prevent having to do that.

πŸ‡«πŸ‡·France nod_ Lille

didn't review everything, what i saw looked much better, thanks :)

πŸ‡«πŸ‡·France nod_ Lille

So something like having a lock set on cache clear, and only serving the assets without writing to the folder when the lock is present?

πŸ‡«πŸ‡·France nod_ Lille

As one of the people tagged in #35 my response in #17 was pretty short, longer version: I'm all for it. I don't think the other want to keep the existing code either. Next step would be to open an issue with a PoC, (this issue is more for meta-talk, i'd rather have the discussion of implementation in it's own non-plan issue). Iterating in contrib is a good idea too, FYI I personally won't have time to be involved outside of core for the next few months.

For the PoC what I have in mind:

  • One or two example of a replacement in core (at least)
  • A list of the various ajax commands we have in core and their feasibility (with a very rough complexity estimate easy/hard) with htmx
  • Test runs don't need to be green, although it' be nice if a couple of them were passing.
  • can we use this to handle ajax form submit? if yes, how complex does it look? would be great to drop our jquery formsubmit fork
  • A big thing here is to improve DX, so a couple of examples of how it could make it easier to introduce some ajax stuff in contrib/projects would be good.
  • rough dependency evaluation https://www.drupal.org/about/core/policies/core-dependency-policies/depe... β†’ (being a great meme poster is not enough :p)
  • BC strategy (support existing calls, or a new codepath altogether, etc.) I'm not too keen on two separate mods. I feel it'll make adoption much, much slower than spending time on the BC layer even if it takes a long time. (we can always put restrictions on what the BC layer can do) if we have 2 code paths, we'd need a contrib module to bridge the two and let contrib maintainers test with htmx easily without changing their code

Feel free to open an issue already, you don't need to have all the above to get started and send code, focus on the fun parts for now :). It's just a list of the things we'll need at minimum on the way to a core commit, it'll probably be split up in several issues once it's mostly ready to make review/commit easier. I checked the code for the contrib module, some stuff could be simplified on the js side (you don't need to attach the eventlistener to the body in a behavior for example) all in all, looks promising.

I'm expecting a lot of discussion in that issue so please have in mind it'll be a marathon, not a sprint. Ajax support in core is ancient and changing it might be challenging, but it's worth it and I'll be here to help along the way.

πŸ‡«πŸ‡·France nod_ Lille

Committed 91b4cf2 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

can I get a rebase/merge of 11.x here please? I know the merge request works but it doesn't apply cleanly as a patch.

πŸ‡«πŸ‡·France nod_ Lille

Committed bd81352 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Same here, the JS is ok with me for a Beta, for stable i want to use proper events like we did for dialog but that can wait (since we'll be marking all the JS apis internal for now), there is generally too much nesting, some things can be refactored but nothing impacting the user experience, it's mostly about DX and maintenance. Better get this feature in users' hands first.

πŸ‡«πŸ‡·France nod_ Lille

Agreed on having everything internal. Can we make the css-only libraries internal too? just in case. We can always make not internal for stable realease.

I would define floating ui as internal too, it's in the assets folder but it's not defined in core libraries hence shouldn't be relied upon just yet. I wouldn't want contrib to start depending on navigation/floating-ui. If we don't want to make that internal, that needs to go in core.libraries.yml

Other than that, +1

πŸ‡«πŸ‡·France nod_ Lille

Thanks! Published the change notice, does it need release note?

πŸ‡«πŸ‡·France nod_ Lille

I agree with the diagnosis and the end goal, and I think shoving components in template is a necessary step.

We do not have a way to site build the components in core yet. We shouldn't wait to have it to make the components so having template call components is a way to let the community start the components and use it in core. When we have the capability to site build all this yes we need to make use of that capability and stop hardcoding the components in template.

πŸ‡«πŸ‡·France nod_ Lille

that's at least major given the impact

πŸ‡«πŸ‡·France nod_ Lille

Committed 20d5c8a and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Committed dc7a429 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

awesome, there was a conflict in libraries.yml on the backport, opened a MR for 10.3.x

πŸ‡«πŸ‡·France nod_ Lille

I was being too cautious limiting to 11.x (was still provisional at the time, might be part of the reason too).

So yeah, 11.x and 10.3.x otherwise it'll be hard to remake a patch

πŸ‡«πŸ‡·France nod_ Lille

got the same results building css locally after a build:css.

-webkit-appearance vendor prefix is fine to remove, it's for old unsupported browsers

πŸ‡«πŸ‡·France nod_ Lille

I'm committing a bunch of issues with css changes to make the RTBC queue go down, so we'll have to reroll that just before commit.

πŸ‡«πŸ‡·France nod_ Lille

Committed 9665396 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

I was worried having drupalSetting added to the preview could break views ui somehow. Haven't found an explicit way it could yet.

Not really my area of the code so leaving the commit to someone else. RTBC +1

πŸ‡«πŸ‡·France nod_ Lille

Committed 299f4c8 and pushed to 11.x. Thanks!

I think that solution is pretty clever, could be good to have in a blog post somewhere :)

πŸ‡«πŸ‡·France nod_ Lille

Committed 3f7d286 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

raising to major since it interferes with the normal use of the site.

We should be looking at potentially splitting the font up to reduce size see https://github.com/zachleat/glyphhanger

πŸ‡«πŸ‡·France nod_ Lille

Committed 296e471 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

about #60 given that this is an automated MR, I'd be reluctant to introduce manual changes on top of it. I did it for the once() replace patch and don't recommend it because of the difficulty to reroll. IMO The comments were placed awkwardly to begin with, so it make sense that an automated fix would place them not where we want.

Given that this is an automated patch, I'm fine with the comments in this issue but a follow-up needs to be created to move them either above or before the opening bracket.

Since we'll use prettier for css, we might want to enable css file rewrites in the prettier command of our package.json. Ok with a follow-up for that since we'll need to exclude a bunch of files/directories from this auto-formatting and it'd make all this longer.

πŸ‡«πŸ‡·France nod_ Lille

all good with me. I'm ok with the variable being in the follow-up, the keyframe stuff was going to be my next question :p let's do that in the other issue.

πŸ‡«πŸ‡·France nod_ Lille

@Pravesh_Poonia I'm not sure what you mean by "But checked MR, changes looking fixed now" . Your text looks automated/generated, could you elaborate as to what you were trying to do? Thanks

πŸ‡«πŸ‡·France nod_ Lille

Minor so no backport

Committed fec3962 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

I was involved with the code so I wanted to have another person RTBC so that I could commit but it doesn't seem to help get that in. It's a 11.x commit only.

πŸ‡«πŸ‡·France nod_ Lille

Awesome, thanks a lot for the clear issue summary and the MR!

πŸ‡«πŸ‡·France nod_ Lille

Maybe for the js-hide class we could use the media query to make sure we hide things asap. Would be good to try it out see if that helps some UI

πŸ‡«πŸ‡·France nod_ Lille

Retitle for commit, opened πŸ“Œ Refactor (if feasible) uses of the jQuery parents function to use vanillaJS Active to address the other uses of parents and change the eslint config

πŸ‡«πŸ‡·France nod_ Lille

The issue is with Js being half-available. if some of the JS execute but not the rest the feature could stay broken, see #3441124-12: Views UI action buttons create janky layout shift on page load β†’ for a possible fix

πŸ‡«πŸ‡·France nod_ Lille

ohh clever, i like that. Don't hesitate to make a class or something out of it. All the MR doing the same thing will need it I think

πŸ‡«πŸ‡·France nod_ Lille

Thanks for the work!

:visible will be tricky to get right so I'd rather leave that in for now, we can always fix it in a follow-up. In some places there is a bit too much of code rewrite, if we just need to update the CSS selector it's better to just do that and not try to replace addClass and that kind of things.

πŸ‡«πŸ‡·France nod_ Lille

Putting back at NW to make sure that in case JS crashes or can't downaload completely this still works.

πŸ‡«πŸ‡·France nod_ Lille

credit doesn't happen automatically for commit if you set it fixed before the commits show up on the issue :)

πŸ‡«πŸ‡·France nod_ Lille

I think the needs review bot should work better.

I agree, not much time for it though

πŸ‡«πŸ‡·France nod_ Lille

looks good to me, bot is just getting in the way. it's not too smart to change the issue major version fast

πŸ‡«πŸ‡·France nod_ Lille

Committed f45c7bd and pushed to 11.x. Thanks!

awesome :)

πŸ‡«πŸ‡·France nod_ Lille

will most likely need a 10.3.x MR since it won't cherry pick :D

Production build 0.69.0 2024