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?
In the meantime related to 📌 More responsive toolbar Active , let's make it directly responsive with a dropdown.
Mark them as noUi is planned when we are Drupal >11.3
Block? I don't think it's a good place, code of this island is already too complex.
We can add something to the label or have an other group than Layout.
Should we have something to mark this island as experimental?
Same for non fully working islands like SSE?
Seems tricky with ENV, Playwright work on an installed Display Builder, every act is manual (or drush).
We need a local/cdn switch in the codebase, either a config, a state, something we can set with drush.
If done on beta we will probably need to introduce a hook_update, so should be done before.
mogtofu33 → created an issue.
mogtofu33 → created an issue.
mogtofu33 → created an issue.
I am doing work of refactoring here #3545303, see my MR.
I created a service for the default provider instead of the unmanageable trait.
We need to remove any DI or extensive logic from `defaultConfiguration()`, It's not meant to have extensive or complex stuff. That's why I removed it.
So the inverted is what I was thinking about too, and I set the sensitive default values only once on the creation of a profile.
My MR is only missing the pass on the one created from config import, but the inverted logic could simplify this part.
At the end this task should be done after #3545303 if not inside instead of separately.
Done in an other merge.
It was fixed. Next perf work to 📌 [1.0.0-alpha2] Component library cache Needs work
Couldn't reproduce in the playwright tests, so was hopeful.
Back to active for someone to fix this.
It was made from the original logo by me, not AI, logo removed, drama avoided.
Off course I vote against the current version.
mogtofu33 → made their first commit to this issue’s fork.
mogtofu33 → made their first commit to this issue’s fork.
Seems ok, to be tested by someone else.
Status is good for now.
Moved to 📌 General bug fixes and code cleanup Active for better visibility and follow up.
mogtofu33 → created an issue. See original summary → .
mogtofu33 → created an issue.
We have a cache problem globally, we need to check if we can now work with js aggregation, and most important with page cache.
For me on some use cases, with page cache, changes on the builder revert to first loaded version because of page cache, need to detail here and fix.
Yes, it's looking like an old cassette.
mogtofu33 → changed the visibility of the branch 3532592-alpha-bug-fixes to hidden.
mogtofu33 → changed the visibility of the branch 3538607-ui-refactor to hidden.
mogtofu33 → created an issue.
mogtofu33 → made their first commit to this issue’s fork.
I don't have the shoelace error and duration is working for me. (at least was...).
But I have other problems with SSE which seems to not work on dev mode (twig dump on), as well when you drop a views (like block Who's online).
Even on a clean install it seems to not work anymore on 1.0.x, but I need to ensure it's not a local problem.
mogtofu33 → created an issue.
Nice work, thanks!
mogtofu33 → made their first commit to this issue’s fork.
Looking at the diff on htmx there's a chance our problems come from attachments processor removal.
For now it makes the UI fail strangely, first drag is working and not second, it fail.
Cannot see for now why. Need investigation.
mogtofu33 → made their first commit to this issue’s fork.
Need a rebase.