mogtofu33 → changed the visibility of the branch 3540610-context-management-for to hidden.
Playwright error is problematic, need to investigate on a new fork.
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.
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.
Add SRI to cdn js: https://www.jsdelivr.com/package/npm/@shoelace-style/shoelace?tab=files&...
This will not trigger an error as we loop only enabled islands.
For clarity let's have a main parent feature issue on tree / layers panel.
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.
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.
I fixed it in bug fix some time ago, to confirm it's ok now.
Playwright test failing seems to show a PHP error, need to investigate to be sure.
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.
mogtofu33 → changed the visibility of the branch 3547972-block-library-feedbacks to hidden.
Will merge the current work as it seems ok and keep the MR open for the follow up.
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.
I centralized to ease this work, I am trying to not use it, but seems tricky, will do more experiment.
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.
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.
Added the preview on blocks that was lost, perhaps it will help.
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
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?
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).
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.
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.
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.
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.
mogtofu33 → changed the visibility of the branch 3545303-island-pass-1 to hidden.
It seems to not really be a problem after all, closing this, will reopen if needed.
I would say not an external module because Layout Builder is a Core module.
As we know layout builder is not really maintained and stay in core only because there is no alternative. Once an alternative is out it will mostly be moved out. And we are one of the credible alternatives.
The navigation module dependency is a bad choice for sure. But dealing with it do not mean embrace it. In our case if entity_view is enabled we will ignore layout builder and inform for the entity_view part. It will still work with navigation.
And off course using Display builder for the navigation could be an option too! It's a bit tricky it seems based on how tight they seems to be to layout_builder but not impossible.
Submodule case
Let's agree it's pretty rare on a Drupal project to carefully enable sub module or be shy to do it, it's generally the opposite, a bunch of not needed sub modules and even modules.
Assuming layout_builder will be enabled on all Drupal (which a lot of project will not like...) sure we have some challenges, and probably work on layout_builder itself to offer changes... But I prefer this work now than doing the tests on this part for us or maintaining this code.
And I am pretty sure the navigation adoption will suffer from this dependency, so we'll have to wait and see.
In `BuilderDataConverter` it seems the layout part can be in a separated service in the external/sub module.
No need in the main module as it's related to entity view, and when display_builder_entity_form is in it can require display_builder_entity_view. Even if someone want only the form part, the fact that the view is enabled by display compensate.
Ok @ipumpkin, I misjudge your involvement.
For sure core HTMX is well and close to be full in 11.3. And we want to be early adopters.
We do not use bareHtmlPageRenderer for all calls, it's used when we don't want the <header> because then the js/css is appended by htmx to the current page.
Hello @ipumpkin and welcome here.
Thanks for the insight but note that without any relevant contribution this will not grant you any credit.
Goal was to defer collecting of keyboard out of the overall build step.
Having the keyboard code in interface is over engineering, I over engineered it at the beginning knowing something was not good. Time to clean.
This logic should not be in islands in the current state. Meaning we have not external usage or manipulation. Let's remove the apiController introduced, and make it definitely as what it is, a js utility to map keyboard to button.
Solution:
An attribute on concerned element, today we have data-keyboard used by keyboard.js to collect the current keys and handle them. We add data-keyboard-help for the help text.
Then it's not the island but the render array anywhere responsible to add the keyboard action by attribute. This has the great advantage of reducing the islands.
Then keyboard.js manage the content of help description text if the button is visible.
The keyboard problem is a bit deeper.
But that's a good opportunity to remove this keyboard code and move it in API with htmx, done here.
The playwright tests detected the keyboard problem, but it's extra local tests not part of test-min for now. Started rewrite a bit but couldn't fully fix it, will check that on follow up.
Until then just need review here and it's ok.
I think the problem is the UIP source views rows is assuming it is used only in views context with views settings available.
But in Display builder we don't have (yet?) the views settings, so using the UIP views rows create an error as it want to get the settings.
So probably this should be a UIP modification?
So I am mostly doing it in https://www.drupal.org/project/display_builder/issues/3545303 📌 Island lighter base class Active , is it ok @just_like_good_vibes?