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

Merge Requests

More

Recent comments

πŸ‡«πŸ‡·France nod_ Lille

Committed 5c30729 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

I tried to find a way to do it as well, didn't find anything obvious. Since using indentEnabled covers most of the obvious cases I'd say it's good enough for now. It'll fix the most user-visible instances of the problem (multi-valued fields) so I'd call that a win.

πŸ‡«πŸ‡·France nod_ Lille

got some issues with the pre-commit hooks (same issue the NR-bot runs into) so I'll be committing that when i figure out the workaround.

πŸ‡«πŸ‡·France nod_ Lille

Perfect just need a rtbc now :)

πŸ‡«πŸ‡·France nod_ Lille

ah yes sorry, you can always add the no-needs-review-bot tag to make him quiet if necessary :)

πŸ‡«πŸ‡·France nod_ Lille

bot is not happy since patch doesn't apply from the MR, can someone merge 11.x in the issue branch so it stops complaining? (for commit we still use the patch, not the MR so it's still important to have the MR working as a patch).

πŸ‡«πŸ‡·France nod_ Lille

We have 2 taxonomy terms with the same name for JavaScript on d.o with different tids, sometimes they get changed automatically depending on which one is used in the autocomplete. it happens.

I'm not sure about adding the parents column conditionally, it sounds like it'll be a headache for backwards compatibility, i'm pretty sure it's not just field_group that make use of this.

πŸ‡«πŸ‡·France nod_ Lille

Committed 223f5c0 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

Need to change the term so that we don't use dummy.

πŸ‡«πŸ‡·France nod_ Lille

phpstan and .api.php files have issues in non-CI runs

πŸ‡«πŸ‡·France nod_ Lille

this was a mistake in πŸ“Œ Refactor (if feasible) use of jquery is function to use vanillaJS Fixed , updated the code to reflect what there before

πŸ‡«πŸ‡·France nod_ Lille

nod_ β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France nod_ Lille

Just wanted to raise a point for Framework Managers about the $schema URL in the components definition.

What we expect folks to add to their metadata yml file is $schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/v1/metadata.schema.json for better DX so that IDEs can provide autocompletion.

I'm not sure pointing to the repo HEAD is the best thing. We can change the schema version by adding a v2 folder and updating the URL, keeping all the versions all the time in the repo.

what I'm not sure is if this URL is going to last 10 years, and if having all the different versions in HEAD is what we want, sounds like it could be somewhere else and not tied to the source of Drupal. Then again it makes sense to have it in the source since that's where all the stuff is defined to begin with.

I don't have a good answer, just questions but I don't think they should hold up this patch.

πŸ‡«πŸ‡·France nod_ Lille

Committed 3923a9f and pushed to 11.x. Thanks!

Not backporting, so that it'll only be released with D11.

πŸ‡«πŸ‡·France nod_ Lille

nod_ β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France nod_ Lille

Posted a solution that should work all the time.

I used a native event so I can listen in the capture phase that way opening the details element is the very first thing that runs, before the native browser callbacks are called, otherwise it causes an error with the details element being open too late.

So use my suggestion and add some more comments please.

πŸ‡«πŸ‡·France nod_ Lille

Why is it that only the ckeditor dll is changed? there is nothing I can review here.

Please post the steps you took to make the change as I can't review an already built file that comes from a third party.

πŸ‡«πŸ‡·France nod_ Lille

Couple of frequent usage case not supported (detail with required field added with ajax, changing the required state with the #states API)

πŸ‡«πŸ‡·France nod_ Lille

The comments were not addressed. A bunch of 'index' parameters needs to be removed, I found a couple more this time too.

πŸ‡«πŸ‡·France nod_ Lille

The parent field is useful when you have a module like field_group that can add a fieldset or something where items can be nested into. Then the parent field does not display unhelpful values.

When field_group is not enabled it does seem unnecessary I agree, the values shown in the select do not seem correct, Content and Disabled are for the region select, not the parent select.

The manage form display (and the other manage display, block placement) are actually hierarchical interfaces with the region being a parent of the rows. I mean technically it makes sense and I agree it's not the user see, just providing some context here.

πŸ‡«πŸ‡·France nod_ Lille

This is what I had in mind.

The block interface, manage display do not seem like hierarchical interfaces but they are in the code (the regions are the first level of hierarchy) so they do not show up as reordering. We can maybe find a way to tweak that but what most people would run into are the multi-value fields to reorder, and those show up properly with the attached patch.

πŸ‡«πŸ‡·France nod_ Lille

Implementation is not going to play nice with modules like field_group that adds possible indentation levels in place they are not possible initially (like the manage display list). Having to add a data attribute (outside of the existing tabledrag config as well) is not a option here. We need the tabledrag script itself to figure this out based on the tabledrag configuration so that we can support contrib properly and not forget edge cases.

Field items are not supported and those would be the most visible ones to update for the majority of users.

Added a few comments in the MR.

Aside from that +1 on the feature :)

πŸ‡«πŸ‡·France nod_ Lille

nod_ β†’ made their first commit to this issue’s fork.

πŸ‡«πŸ‡·France nod_ Lille

need some validation from olivero maintainers for the size change.

Need to update the screenshots in the issue summary as well.

πŸ‡«πŸ‡·France nod_ Lille

Thanks for the efforts, I know those type of issues can be frustrating so thanks for sticking with it :)

Committed 4e26ae9 and pushed to 11.x and 10.3.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

can someone reup a path that applies or better yet a MR?

thx

πŸ‡«πŸ‡·France nod_ Lille

Have a question about the vertical align change. It's pretty overkill. I think we should adjust the fontsize/padding of th in the table body instead. Like this we'll mess up layout for tables with cells that are not the same length and it could make them very hard to read.

πŸ‡«πŸ‡·France nod_ Lille

Committed b4a0a9e and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

I'd rather remove the animation altogether. By now we can use CSS to do animation if necessary. Those two places are pretty old things. I don't think the animation adds anything in term of design or usability, especially since our dropbutton does not have an animation and there is one right next to the views +add button.

πŸ‡«πŸ‡·France nod_ Lille

few things left, the change record needs more details

πŸ‡«πŸ‡·France nod_ Lille

It's solid work, a couple of comments. Mainly about keeping the settings value alterable and using a subclass of Event instead of CustomEvent everywhere.

Setting NW for feedback and the change record.

πŸ‡«πŸ‡·France nod_ Lille

yes, consulted with other core committers we're moving the word to drupal-dictionary.

see πŸ“Œ Keep the word dependee and move it to drupal-dictionary.txt Active

πŸ‡«πŸ‡·France nod_ Lille

confirmed we don't have code using jquery .when (outside jquery ui at least)

Committed 56bf8c1 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

#46 is correct, the BC layer is missing.

I'm not native and dependee makes sense to me (like employer/employee) so to take with a grain of salt: I'd be for leaving this alone given the work needed to make it work properly (depreciation, bc layer, etc.)

πŸ‡«πŸ‡·France nod_ Lille

Seems a forced-color image has been removed, can someone check in forced color mode? thx.

πŸ‡«πŸ‡·France nod_ Lille

Used #25 for commit.

Committed 8ab1f3c and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

has anyone tried the suggestion in #3? Having the items centered is not better than the current situation. I would expect the tabs to be aligned at the bottom so that the highlighted border is always against the white part of the page.

With the current patch the border floats in the middle for short menu items like "edit".

πŸ‡«πŸ‡·France nod_ Lille

Problem with the float value, postcss and RTL styles.

πŸ‡«πŸ‡·France nod_ Lille

Committed 9e0ed31 and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

+1 on the idea.

I'm not super sold on the green tick (at least that show up as a green check on my system) since the comments are already green by default on FF/chrome and the black square stands out better. But it's just cosmetic stuff and it is better than it was so i don't mind enough to change it.

For "πŸ’‘ BEGIN CUSTOM TEMPLATE OUTPUT" it should also update the end output comment no: "END CUSTOM TEMPLATE OUTPUT", I don't think we need the emoji for this one.

πŸ‡«πŸ‡·France nod_ Lille

Committed ffcce9c and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

does the minified file looks ok? maybe the css minifier is having trouble with the comment?

If you remove the comment, does it looks ok?

πŸ‡«πŸ‡·France nod_ Lille

The perf improvement is nice, but i'm worried about the sustainability of bun, also doesn't really work on window yet.

πŸ‡«πŸ‡·France nod_ Lille

This is becoming off topic. Also this is something that can fully be done in contrib, you're describing a mix of adv_agg and jquery_update.

πŸ‡«πŸ‡·France nod_ Lille

Committed ce9604c and pushed to 11.x. Thanks!

πŸ‡«πŸ‡·France nod_ Lille

We do have a whole thing for third party js deps so we don't have to copy/paste manually:

$ yarn add -D jquery@4.0.0-beta
$ yarn vendor-update

I'll get the code in the right place and update the version in core.libraries.yml

πŸ‡«πŸ‡·France nod_ Lille

I was on the fence, but our php validation does consider blank spaces as empty

πŸ‡«πŸ‡·France nod_ Lille

few details, i didn't point out all the places it could be simplified, just gave a couple of examples.

πŸ‡«πŸ‡·France nod_ Lille

If we hit some limits of the import map api, An alternative solution could be reaching into the js source and rewrite import paths at process/bundle time.

I'm worried that the import map will end up being massive for something like Drupal. Maybe not, we'll see when we get there I guess.

Do we need to worry about security if we have an import map with everything? I know we don't have access rules on js assets by design so it shouldn't be an issue but you never know.

πŸ‡«πŸ‡·France nod_ Lille

Committed 6249892 and pushed to 11.x. Thanks!

backported to 10.2.x

πŸ‡«πŸ‡·France nod_ Lille

and to anwser the questions.

The problem is not present in 10.2 because it doesn't have the refactor, 10.2 version was used as an example of what the code and behavior should be.
It is probably not possible to replicate with core since we got rid of dropbuttons on those kind of submits a while back.

πŸ‡«πŸ‡·France nod_ Lille

Committed 39fb7eb and pushed to 11.x. Thanks!

I opted to commit it from here since this is a simple mistake and it should have been part of the initial patch. I do not see it as a sign there is a deeper issue with the previous patch.

πŸ‡«πŸ‡·France nod_ Lille

There are several places that create a jquery object just to get the original dom element after. We can get rid of the creation of the jQuery object and use the dom directly

πŸ‡«πŸ‡·France nod_ Lille

The MR is just a search/replace and no code has been run apparently, closing the MR to avoid confusion, it needs to be reworked from scratch.

Please create a new branch from 11.x and start a new MR

πŸ‡«πŸ‡·France nod_ Lille

this does not seem like it would work. calling classlist on a jquery collection won't do much.

πŸ‡«πŸ‡·France nod_ Lille

The replacements do not look straightforward, an intermediate method could be used to make is nicer and handle the various edge cases we're most likely missing, but at that point why not just use jQuery itself?

I'm on the fence here, I'd go with leaving that as-is and revisiting once we're closer to removing jquery.

πŸ‡«πŸ‡·France nod_ Lille

Migration code is in a rough shape but don't have more time for it now, next will be to handle the secret validation like user password were handled for the core D7-8 migration

πŸ‡«πŸ‡·France nod_ Lille

thanks! fixed that one.

Hopefully the selenium one won't be too bad…

πŸ‡«πŸ‡·France nod_ Lille

Need help with the test failures here.

First is the test testAdvancedCaching I do not have a clue why this happens, fails with

 Caused by
 ErrorException: Method "IteratorAggregate::getIterator()" might add "\Traversable" as a native return type declaration in the future. Do the same in implementation "Drupal\Core\Entity\ContentEntityBase" now to avoid errors or add an explicit @return annotation to suppress this message.

And the rest of the failures happens because selenium doesn't seem to think that our custom element can be interacted with. Haven't found out why yet.

πŸ‡«πŸ‡·France nod_ Lille

Accessibility is not an issue with this one. We don't use the shadow dom, the markup is stricly the same as before inside, and the drupal-dropbutton replaces a div which has no special meaning either.

πŸ‡«πŸ‡·France nod_ Lille

At this point I'm looking for feeback on the overall approach. Chose dropbutton because it's self contained, and it's not related to forms. Other webcomponents that could be interesting: autocomplete, marchine name, vertical tabs, time diff, etc. anything self-contained that uses behaviors for init.

This makes dropbutton jquery-free as a side effect.

This makes core use the defer attribute, which will prevent a few things from working regarding aggregation at the moment, see πŸ› JavaScript aggregation should account for "async" and "defer" attributes Needs work .

πŸ‡«πŸ‡·France nod_ Lille

nod_ β†’ created an issue.

πŸ‡«πŸ‡·France nod_ Lille

I like this a lot.

One major concern is the renaming of the event, I think we can avoid having to rename it because that would be a pain for contrib to deal with.

I'd like to remove only the .trigger call, not addevent listeners yet. That will allow people to use dom events without breaking their existing code. Then we can remove jquery.on afterwards.

πŸ‡«πŸ‡·France nod_ Lille

I don't think we can pass whatever corepack enable does as an artifact. Probably a before script makes more sense, I don't have strong preference, whatever works is good with me. I'm more concern of existing devs not having this working. I think we need the yarnPath if an existing yarn exec already exists. Can someone test it?

πŸ‡«πŸ‡·France nod_ Lille

all right, wanted to make sure we could use yarn with corepack only, it's simpler than #54 because we have a package.json with a packageManager entry.

Using yarn like this present some issues:

Not sure if we should go with this or with the executable committed to the repo, in any case it works as expected.

πŸ‡«πŸ‡·France nod_ Lille

Can you replace the domain but keep the rest of the URL intact please? also in the test no need to test for the whole link tag.

So just like it was before, and replacing the google domain with example.com

Production build https://api.contrib.social 0.61.6-2-g546bc20