Problem/Motivation

Be able to add classes to theme regions.

Store in theme settings. (Or should it be handled by UI Skins?)

Issue fork ui_styles-3353493

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

Grimreaper created an issue. See original summary.

elber’s picture

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

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

kevinoliva’s picture

Assigned: Unassigned » kevinoliva

grimreaper’s picture

Assigned: kevinoliva » grimreaper
Status: Active » Needs work
seda12’s picture

Assigned: grimreaper » seda12
seda12’s picture

Assigned: seda12 » Unassigned
Status: Needs work » Needs review
grimreaper’s picture

Assigned: Unassigned » grimreaper
pdureau’s picture

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:

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...

grimreaper’s picture

Assigned: grimreaper » pdureau

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.

pdureau’s picture

- 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 :)

grimreaper’s picture

Assigned: pdureau » grimreaper
grimreaper’s picture

Title: Add ui_styles_regions sub module » Add ui_styles_region sub module
grimreaper’s picture

Title: Add ui_styles_region sub module » Add ui_styles_page sub module
grimreaper’s picture

@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.

grimreaper’s picture

Status: Needs review » Needs work

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

grimreaper’s picture

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.

grimreaper’s picture

Assigned: grimreaper » pdureau
Status: Needs work » Needs review

@pdureau, I had created the child issue #3379246: Make StylePluginManager::alterForm theme contextual 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.

grimreaper’s picture

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.

grimreaper’s picture

Assigned: pdureau » grimreaper

Discussed with @pdureau.

I will test on ui_suite_bootstrap the navigation_collapsible region.

grimreaper’s picture

Assigned: grimreaper » pdureau
StatusFileSize
new19.09 KB

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.

grimreaper’s picture

Assigned: pdureau » grimreaper

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.

  • Grimreaper committed 4410379b on 8.x-1.x
    Issue #3353493 by Grimreaper, kevinoliva, seda12, pdureau: Add...
grimreaper’s picture

Assigned: grimreaper » Unassigned
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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