Problem/Motivation
We use preprocess hooks in UI Suite themes and it appears we are always doing the 2 same stuff:
- Adding the theme base path for preview content with images or videos. Example: _ui_suite_protocol_add_media_path().
- Adding some HTML class or other attributes to a children component. Examples: Drupal\ui_suite_bootstrap\HookHandler\PreprocessPatternCard::addCardImageClass(), _ui_suite_protocol_add_link_class.
Preprocess are PHP function and are not expected to be written by the front developer, so they reduce the ownership of the design system maintainer, so let's remove them.
Proposed resolution for: the theme base path
We can take inspiration from ECA module and its use of tokens: https://www.drupal.org/project/eca/issues/3278918
However, there is token to get theme path in Drupal Core and we may need take inspiration from: https://www.drupal.org/project/theme_filter
card:
label: Card
fields:
media:
label: Media
preview:
- type: html_tag
tag: img
attributes:
src: "[path-to-theme:absolute]assets/card.png"
Proposed resolution for: adding attributes to a children component
It may be managed by #3301853: Create twig filters: |add_class and |set_attribute so we need to evaluate those new filters.
We can also introduce new properties (add_class, set_attribute...) and namespacing in pattern definitions. Example:
card:
label: Card
fields:
action_buttons:
type: render
add_class:
- mdc-card__action
set_attribute:
role: action
preview:
- type: "pattern_preview"
id: "button"
- type: "pattern_preview"
id: "button"
Issue fork ui_patterns-3311480
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
Comment #2
grimreaperLet's use set_attributes and not add_class to ensure to have this level of genericity.
Comment #3
g4mbiniComment #4
pdureau commentedComment #5
pdureau commentedComment #6
pdureau commentedEvaluation of the 2 filters proposed by #3301853: Create twig filters: |add_class and |set_attribute and planned for Drupal 10.1.
They work correctly only when the twig variable is a single render array, and not a list of render arrays (which is a very common use case).
So, I will propose un patch for https://git.drupalcode.org/issue/drupal-3301853/-/blob/3301853-create-tw... to loop on array items is the array is a list:
If this patch is accepted, there is nothing to do on ui_patterns side.
For example, for https://git.drupalcode.org/project/ui_suite_bootstrap/-/tree/4.0.x/templ... we can replace the preprocess by the add_class filter:
Comment #7
pdureau commentedComment #8
pdureau commentedMaybe the 2 new Drupal 10.1 Twig filters are not such a good idea, even if our patch is accepted.
A lot of filters are "typed": they must be applied on specific types. Examples:
We tests several Twig filters on UI Patterns fields ("slots"), applying them to the wrong type:
We are fortunate the UI Suite themes are currently only using filters on settings ("props"),not on "fields" ("props")
Except DSFR, but there is a ticket about that: #3341139: [beta1] ⚠️ Fix compatibility with layout builder
We can therefore promote this ("no filters on slots") as an official good practice
So, let's explore the new YAML definitions properties (add_class, set_attribute...) again.
Comment #10
pdureau commentedFirst MR: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/22
It is just an humble proposal for now. Let's discuss.
Wih this MR, when we have this markup to integrate:
In order to inject the figure-img class to the children render element, we can avoid an hook like ui_suite_bootstrap_preprocess_pattern_figure by adding the class into the definition YAML:
Comment #11
pdureau commentedFeel free to share feedbacks
Comment #12
pdureau commentedComment #13
pdureau commentedBecause we are considering filters on slots again in #3342390: Make the Twig loops safer we can propsoe a Twig filrer instead of altering the render element.
Instead of relying on #3301853: Create twig filters: |add_class and |set_attribute which will have those issues:
We implement our own versions in UI Patterns:
So :
Comment #15
pdureau commentedNew MR: https://git.drupalcode.org/project/ui_patterns/-/merge_requests/25
First commit, 1e4d214a3d9 : Add set_attribute and add_class filters, exactly as proposed in #3301853: Create twig filters: |add_class and |set_attribute, included the unit tests.
Second commit, 7874e0c5e90 : with expected changes for UI Patterns:
Compliant with feedbacks from Grimpreaper:
This MR replace the one from comment #10, and its branch ("3311480-remove-the-need") replace "pierre_proposal" branch.
Comment #16
pdureau commentedI rename the issue, because we haven't deal with the theme base path subject for a long time. Let's create a new specific issue if there are some progress.
Comment #17
grimreaper@pdureau thanks for your work in this issue.
Maybe an additional test to add and some minor comments to update.
For the PHPCS part, I will do it when the other review points are done.
Comment #18
pdureau commentedDone.
2 new tests:
PHPCS is OK:
Comment #19
pdureau commentedComment #20
grimreaperComment #21
pdureau commentedFinally, I have removed TwigExtensionTestString which is not used for testing.
Tests are still OK:
PHPCS is still OK:
Comment #22
grimreaperSome review comments remains.
Comment #23
pdureau commentedComment #25
grimreaperMerged on 8.x-1.x. Cherry-picking on 2.0.x.
Comment #29
grimreaperMerged on 2.0.x too.
Comment #31
sharique commentedDocumentation and examples update is required for this.