Account created on 17 April 2008, over 17 years ago
#

Merge Requests

More

Recent comments

🇫🇷France mogtofu33

I don't provide grouping or sorting because the discovery is already performance sensitive, so it needs to be done carrefully.

The Icon Plugin Manager is in Drupal core for Drupal 11.1+ the plugin manager id is icon_pack for alter as you can see in core/lib/Drupal/Core/Theme/Icon/Plugin/IconPackManager.php so you can use hook_icon_pack_alter().

🇫🇷France mogtofu33

Could not reproduce, need more information.

🇫🇷France mogtofu33

I would say for a social icon block un the footer:

  • Enable UI Icons Menu
  • Create a Social Menu with icons and links
  • Set the menu block in the footer
  • Enjoy?
🇫🇷France mogtofu33

It's more that the current modal picker is hard to maintain and I don't have time to maintain it. The autocomplete grid picker doing the same in more simple way. So I will probably move it as contrib so anyone can help and maintain it.

You can already achieve a limited selection of icons by using Media with UI Icons Media, then you are able to use all media library feature to allow selection of a selected set of icons. It's better because then you have benefit from other contrib modules for the widget selector.

🇫🇷France mogtofu33

Seems like your forgot in your steps to specify your field widget configuration (Manage form display) is "Picker" and not autocomplete.
For now Picker is not compatible with field default value, as the Autocomplete grid provide the same functionality in a more simple form, it's planned to replace the picker field which is too complex in comparaison.

So just change the field widget to autocomplete with grid, it should work.

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

Added label and icon visibility to some toolbar buttons.

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

The agile way I would like to follow is based on feature status and bugs, dark mode is fully complete with no bugs...

Navigation will be core, but can be disabled, if disabled we do not work. If we mark it as a dependency, if people want to use admin toolbar this will not work. And for reference admin_toolbar has 300k sites declaring using it.

🇫🇷France mogtofu33

mogtofu33 created an issue.

🇫🇷France mogtofu33

Gin admin 5.x support dark mode, 6.x for core seems to need some work 🌱 [META] Gin 6.x Active , as gin toolbar is removed I don't see dark mode for the navigation but it will mostly be in the theme.
IF so I guess we should have it. Problem is we have it too soon before we have gin+navigation in core.

Big problem is what do we do when navigation is disabled? The fact that we throw away any support for toolbar or admin toolbar is problematic for me.

Overall seems too soon to fully embrace navigation, with few benefit as the MR is adding double more code than what it remove.

🇫🇷France mogtofu33

mogtofu33 changed the visibility of the branch 3540610-context-management-for to hidden.

🇫🇷France mogtofu33

Playwright error is problematic, need to investigate on a new fork.

🇫🇷France mogtofu33

There is a strange issue with Playwright test, but not sure it's related. So let's go with this, added a minimum test and some renaming.

🇫🇷France mogtofu33

Wrapped the css in .ck-content and added pictogram and icons colors.

For sure this kind of route seems strange in ui_styles, and dynamic prefix would be huge as dsfr is huge (The PHP process to wrap is heavy)
I tried a packed solution with prefix to include (with gulp for example), but there is some co lateral problems.

The current dirty fix seems enough for me.

🇫🇷France mogtofu33

mogtofu33 created an issue.

🇫🇷France mogtofu33

Will fix the helper and add unit test.

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

This will not trigger an error as we loop only enabled islands.

🇫🇷France mogtofu33

Thanks for the fix!

🇫🇷France mogtofu33

For clarity let's have a main parent feature issue on tree / layers panel.

🇫🇷France mogtofu33

Being able to call an api endpoints when you have the main permission, even if the profile do not expose the island in the UX is not a security problem, just a restriction of functionality problem.
Because of complexity introduced and load we should only be concerned by 'view' and 'save'.

Access to view/edit to the parent entity concerned is enough to decide the view and save instance. (We have no delete the parent entity, that's good).
Save button permission check should be done on the related save function for each entities at the time of saving.

Save preset as well could be check and the save part.

🇫🇷France mogtofu33

Probably serialization problem with Context classes.

That's strange from fresh install I still have the entity context even with no context in the preset. Tested with db_theme_test and component test_simple.

🇫🇷France mogtofu33

Fixed the naming of the hook class.

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

I fixed it in bug fix some time ago, to confirm it's ok now.

🇫🇷France mogtofu33

Playwright test failing seems to show a PHP error, need to investigate to be sure.

🇫🇷France mogtofu33

Fixing the second drawer opening I realize with your secondDrawer.label = I could simply set the title.
For first sidebar as well, pretty easy to set the button label.
The difficult part was on drag, but I managed it in js as well.

Opening a branch for this js only, thanks for the work @sea2709.

🇫🇷France mogtofu33

mogtofu33 changed the visibility of the branch 3547972-block-library-feedbacks to hidden.

🇫🇷France mogtofu33

Will merge the current work as it seems ok and keep the MR open for the follow up.

🇫🇷France mogtofu33

Cannot reproduce on the navigation menu block, I guess its "Main Navigation"?
If so any other contrib module doing something on the menu block?
As you can see in the backtrace I don't really handle the render, UI Patterns is doing, perhaps there is some options missing as we have none for preview.

🇫🇷France mogtofu33

I centralized to ease this work, I am trying to not use it, but seems tricky, will do more experiment.

🇫🇷France mogtofu33

I didn't have time to check the code, but when I am proposing more js side, note that it's because we already have the component name in the DOM, it's the attribute `data-node-title`. I am using it to display the current component name in the right click menu. So I guess we could achieve the same for the end drawer.

🇫🇷France mogtofu33

Added a fix to not store empty values for exclude (and status).

Moved the choices group part of block as a helper because it was pretty hard to understand. Added a Unit test on it.
I changed the groups process with some new ones, and no more empty, with an arbitrary weight. @grimreaper if you can quickly review that it's still ok with your blocks? I put unknown at the bottom but perhaps could be higher.

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

Good for me!

🇫🇷France mogtofu33

Added the preview on blocks that was lost, perhaps it will help.

🇫🇷France mogtofu33

I prefer 'back' it makes more sense.

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

First we could start with a simple exclude block by id like I did for component here, could even support regular expressions.

The opposite behavior 'include' could potentially bring some problems:

  • Called on each build() and heavier to process
  • A new block added after by a module or code or block content should be included by default and not excluded
🇫🇷France mogtofu33

First quick test and review. I fixed some style, name or logic issues.

Seems to work fine, as a detail I always got 'Entity' as context even if it's a preset without any context (created from devel instance for example). Which seems strange.

Then I see that the PatternPreset::getContexts() is called each time the library of presets is loaded for each preset (even the non compatible ones).
It seems expensive, could the contexts be stored on the entity the first time and when edited only? So it can be cached. I tested with full entity cache to ensure it was not cached anywhere.

An other unrelated issue that I thought was fix is the preset not persisting in the builder:
Create a preset, add the preset to the builder: it's rendered, but then refresh and the preset is not here anymore.
Can you confirm this problem?

🇫🇷France mogtofu33

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

🇫🇷France mogtofu33

Extra field can make sense, for example entity links, we could just have a source for this one.
Here problem is modules with extra field will not be available, which is probably an issue (like flag, and probably many other).

🇫🇷France mogtofu33

Thanks for the work on that, as I mentioned on comment #10 📌 [1.0.0-alpha1] Replace "Settings" by an InstanceTitle island Active , if possible I would prefer to investigate a full js solution. Perhaps it's not possible but I see a lot of effort here and I don't want to have it wasted at the end.

🇫🇷France mogtofu33

at the end it's a global percentage on your whole codebase.

Unfortunately, I never got a real occasion of using test/code coverage with reports. So if I understand correctly, even if possible to mark file, class or function as excluded from test coverage, it will still impact the result like never reaching 100% ?

You can ignore but then you can run phpunit coverage without the ignore and people realize you are lying on your code coverage...
Unless you say 60% of core stuff, but again it's a tricky way of saying, I would not lie that.

Code related it's simple, if there is 60% coverage it can be in the beta codebase, if not it's an external module.

I don't understand the "Code related it's simple" or is it our French speaking habits which do not translate well? :)

Where does the "60%" come from?

I mean the code can be there for beta if it's covered at 60%, and it's the lowest level.

For 60% there is multiple factor to help determine how much you want for your code.

There is PHPunit grading you can see here at the bottom. 60% is just above medium.

Then the standards in software dev to demonstrate your code is tested is 60-80%, people selling tests tools will say 75% some other selling just tools around will say 80%.

And off course our friend AI will say:

General PHP / Industry Benchmarks

70–80% coverage is often cited as a good target for maintainable projects.

100% coverage is not realistic or necessary — it doesn’t mean 100% of cases are tested, just that every line was executed once.

What matters more is testing critical paths and edge cases (business logic, security, APIs), not just raw percentage.

we are professionals

I don't know if there are a lot of contributors or people who use Drupal nowadays who are not professional (sorry, assumption...). So not sure if this argument is still relevant or maybe this word is not the most accurate.

The "we are professionals" is related to the term Pierre is using on docs and slides. To promote the quality of what we are doing.

So if it's a statement of trust of quality, be backed by metrics is the best way!

I agree with that even if I am not sure the percentage of people sensitive to this argument, but at least for me it is a good argument. :)

Depends the people, the one I target are those who even if they don't know exactly what it means know it's a good argument to get a green light on a real world project.

And to finish, most Drupal modules go from 0 to 30% coverage, but even if there is worst, I can assure we are not an example in the industry. And I am not talking about 'older' mature languages in SSII where 80% is standard.

🇫🇷France mogtofu33

A little note on code coverage, at the end it's a global percentage on your whole codebase.

If we want to be serious and avoid problems, a target around 60-80% is credible. And in that we can focus on 90% for code we consider critic.
So in this way it can compensate with low coverage on some parts, but low always impact the overall.

In the meantime, if the layout_builder related code is not well tested, this will affect the 'smooth' transition we want to offer... We say yes it works with it, and then there's a bug and it's not working... We fix, an other bug..., we fix and introduce an other bug... Tests will not give full assurance it work in all cases, but it help a lot and is way better than no test.

Code related it's simple, if there is 60% coverage it can be in the beta codebase, if not it's an external module.

🇫🇷France mogtofu33

Seems not possible in the current implementation.

The toast need an action on the page to show and the wrapper, but the current alert message is from htmx, and chaining the action to show the toast do not work.

🇫🇷France mogtofu33

mogtofu33 changed the visibility of the branch 3545303-island-pass-1 to hidden.

Production build 0.71.5 2024