Add ui_styles_page sub module

Hi please can you give me more details about this issue?

Hello all,

thanks for this feature which looks amazing.

2 feedbacks:

ui_styles_regions sub module

I don't think a ui_styles_regions sub module is a good idea.

We will have those modules:

• ui_styles_layout_builder
• ui_styles_regions
• ui_styles_block_layout (from ✨ Add ui_styles_block sub module Fixed )

A few months ago, we talked about reorganizing modules around plugins types instead of back-office tools, so splitting ui_styles_layout_builder between ui_styles_blocks and ui_styles_layouts.

If we do so, we can have:

• ui_styles_blocks which will work for both "block layout" and "layout builder"
• ui_styles_layouts which will work for "layout builder" and anywhere layouts are used. Because regions are related to layouts, this feature belongs there.

Attributes manipulations

Are we sure there is always an "attributes" variable in the render array manipulated by src/HookHandler/PreprocessRegion.php ?

There are plenty of example of regions without specific wrappers, just hosting one or more blocks. For example, when layouts plugisn are generated from UI Patterns. In this case, we need to attach the styles to children available to host an attributes objects.

That's why we created https://git.drupalcode.org/project/ui_styles/-/blob/8.x-1.x/src/Render/E...

And we have added some specific methods to: https://git.drupalcode.org/project/ui_styles/-/blob/8.x-1.x/src/StylePlu...

Hi,

@pdureau, I would suggest:
- ui_styles_block
- ui_styles_layout (for layout regions)
- ui_styles_region (for theme regions)

No ending "s". There is ui_styles_views because core module name is "views".

I don't think theme regions and layout regions are the same concept. Are you ok with that? I am opened for discussion.

I will give a look about the attribute warning for regions during the review.

- ui_styles_block
- ui_styles_layout (for layout regions)
- ui_styles_region (for theme regions)

Proposal:

• ui_styles_block
• ui_styles_layout (for layout regions)
• ui_styles_page (for page container & theme regions)

No ending "s". There is ui_styles_views because core module name is "views".

Indeed :)

@pdureau, regarding comment 10 about your question if attributes is always present.

As in the default region template provided by the system module, there is a wrapper using the attibutes, I would say we only have to cover this case and if in a theme someone is not using attributes in all regions or individual regions, I would say it is this theme responsability to do its own region preprocess and then use the style manager to the styles to the region content.

Todo:
- update tests
- ensure tests covers config
- code quality

When writing tests, it makes me realised that StylePluginManager::alterForm(), is not contextual of the theme, and so is getGroupedDefinitions().

I will skip this for now. But I think the methods signature and implements will have to be changed.

@pdureau, I had created the child issue ✨ Make StylePluginManager::alterForm theme contextual Closed: duplicate but finally implemented it directly here.

I would like your review on the MR.

If you are ok with that I will create the change record for the new method parameter.

To keep track of a discussion I had 2 days ago with @pdureau.

We agreed about the fact that it is a theme owner responsibility to ensure the theme's regions Twig templates are using the attributes object and otherwise to provide automatic injection into the region's content otherwise.

If I don't remember well our discussion, please post correction.

Currently in UI Suite Bootstrap there is one region not using the attributes https://git.drupalcode.org/project/ui_suite_bootstrap/-/blob/5.0.x/templ..., we may want to open a follow-up issue in UI Suite Bootstrap to handle that.

And I guess the following template overrides may be removed after this issue:
- https://git.drupalcode.org/project/ui_suite_bootstrap/-/blob/5.0.x/templ...
- https://git.drupalcode.org/project/ui_suite_bootstrap/-/blob/5.0.x/templ...

Still keeping the issue assigned to @pdureau for review.

Discussed with @pdureau.

I will test on ui_suite_bootstrap the navigation_collapsible region.

So for regions which do not use the attributes variable in the Twig template, there is no way to inject the classes in content because it is already rendered when using hook_preprocess_region.

We tried with @pdureau to workaround the early rendering there is before hook_preprocess_region but we would end up having more and more side effects.

So deciding in ui_suite_bootstrap to remove the navigation_collapsible region from the regions styles form but a theme cannot alter a form of the admin theme...

So keeping as it is.

I will do a last check then merge.

Automatically closed - issue fixed for 2 weeks with no activity.