- Issue created by @pdureau
- @just_like_good_vibes opened merge request.
- 🇫🇷France just_like_good_vibes PARIS
i am currently converting, i have several questions :
- do we keep the slot name "main" instead of "first" for slot name? it would help backward compatibility but this is not straightforward to have a column slot named "main" when it is just the first one. i kept the name main for now.
- we originally have a lot of html classes for thegrid--layout.html.twig
file. i feel like they are not required here. i kept them for now.
- There may be a misunderstanding in the container classes. When i read the doc, it seems like classesfr-container
andfr-container--fluid
may be exclusive. why having two props then ? i kept two propswith_container
andfluid_container
to have some backward compatibility but updated the twig file to have{% if with_container or fluid_container %}
- 🇫🇷France just_like_good_vibes PARIS
I found numerous bugs on the actual codebase when trying to render the example page.
therefore i did a few minor adaptations directly in this MR, even if it is not the right place.
You prefer to split into several issues? - 🇫🇷France pdureau Paris
Can you rebase?
Also, did you remove
template/layout--grid.html.twig
?The changes on
fr-icon-*
classes would be better in a distinct issue.do we keep the slot name "main" instead of "first" for slot name? it would help backward compatibility but this is not straightforward to have a column slot named "main" when it is just the first one. i kept the name main for now.
You can chose "first" if you prefer
we originally have a lot of html classes for the grid--layout.html.twig file. i feel like they are not required here. i kept them for now.
The HTML classes need to be compliant with upstream documentation: https://www.systeme-de-design.gouv.fr/fondamentaux/grille-et-points-de-r...
We don't care about the rest. For example, you can skip:
'layout', 'layout--' ~ layout.id|clean_class,
There may be a misunderstanding in the container classes. When i read the doc, it seems like classes fr-container and fr-container--fluid may be exclusive. why having two props then ? i kept two props with_container and fluid_container to have some backward compatibility but updated the twig file to have {% if with_container or fluid_container %}
You are right, a single prop with an enum would be better.
- 🇫🇷France pdureau Paris
do we keep the slot name "main" instead of "first" for slot name? it would help backward compatibility but this is not straightforward to have a column slot named "main" when it is just the first one. i kept the name main for now.
I understand the change you did for
grid_row_2
,grid_row_3
andgrid_row_4
. But is it expectedgrid_row_1
asfirst
slot as only slot? Just asking the question... - 🇫🇷France just_like_good_vibes PARIS
good question. i think it is simpler to have the same internal names between all the four components... we can keep the label main for the single column case :)
i will correct the labels and repush
- 🇫🇷France pdureau Paris
I will take time doing this review because, looking at your props, I may have found a little something to change in UIP2 prop types
- 🇫🇷France pdureau Paris
(Sidenote for later: why prefixing so much props with "fr_"?)
- 🇫🇷France just_like_good_vibes PARIS
it was like that in "grid_row" existing component, and in the layout options. so i kept the same naming.
but clearly we can remove the prefixes
- 🇫🇷France pdureau Paris
This is something we will need to change. And it will be the good opportunity to update grid_row too. But for now, we do nothing until I fixed the UIP2 issue
- 🇫🇷France pdureau Paris
So, we may need this UIP2 issue to be completed first: 📌 [2.0.0-beta5] Add new EnumSetPropType Active
Because, once coupled with the new SelectsWidget source, the
enum_list
prop may be a perfect fit for list of values based on the same enum.For example, those 10
enum
props:fr_col_first: title: "Column span first" type: string enum: [fr-col-1, fr-col-2, fr-col-3, fr-col-4, fr-col-5, fr-col-6, fr-col-7, fr-col-8, fr-col-9, fr-col-10, fr-col-11, fr-col-12] fr_col_first_sm: title: "Column span first (small)" type: string enum: [fr-col-sm-1, fr-col-sm-2, fr-col-sm-3, fr-col-sm-4, fr-col-sm-5, fr-col-sm-6, fr-col-sm-7, fr-col-sm-8, fr-col-sm-9, fr-col-sm-10, fr-col-11, fr-col-sm-12] fr_col_first_md: title: "Column span first (medium)" type: string enum: [...] fr_col_first_lg: title: "Column span first (large)" type: string enum: [...] fr_col_first_xl: title: "Column span first (extra large)" type: string enum: [...] fr_col_second: title: "Column span second" type: string enum: [fr-col-1, fr-col-2, fr-col-3, fr-col-4, fr-col-5, fr-col-6, fr-col-7, fr-col-8, fr-col-9, fr-col-10, fr-col-11, fr-col-12] fr_col_second_sm: title: "Column span second (small)" type: string enum: [fr-col-sm-1, fr-col-sm-2, fr-col-sm-3, fr-col-sm-4, fr-col-sm-5, fr-col-sm-6, fr-col-sm-7, fr-col-sm-8, fr-col-sm-9, fr-col-sm-10, fr-col-11, fr-col-sm-12] fr_col_second_md: title: "Column span second (medium)" type: string enum: [...] fr_col_second_lg: title: "Column span second (large)" type: string enum: [...] fr_col_second_xl: title: "Column span second (extra large)" type: string enum: [...]
could be replaced by those 5
enum_list
props:cols: title: Cols type: array maxItems: 2 items: type: integer enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] cols_sm: title: Cols SM type: array maxItems: 2 items: type: integer enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] cols_md: title: Cols MD type: array maxItems: 2 items: type: integer enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] cols_lg: title: Cols LG type: array maxItems: 2 items: type: integer enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] cols_xl: title: Cols XL type: array minItems: 2 maxItems: 2 items: type: integer enum: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12]
- 🇫🇷France pdureau Paris
Side note: on the actual MR (so before my proposal with
enum_list
, offset values were wrong it some enumsfr-col-<code> instead of <code>fr-col-offset-
- 🇫🇷France pdureau Paris
Proposal based on 📌 [2.0.0-beta5] Add new EnumSetPropType Active : https://git.drupalcode.org/project/ui_suite_dsfr/-/merge_requests/122
If you are OK with this proposal, let's discuss before merging:
- Because 📌 [2.0.0-beta5] Add new EnumSetPropType Active neeeds to be merged before
- We also need to update
grid_row
to the new structure
- @pdureau opened merge request.
- 🇫🇷France pdureau Paris
We also need to update grid_row to the new structure
Ok, let's do it.
- 🇫🇷France pdureau Paris
Done: https://git.drupalcode.org/project/ui_suite_dsfr/-/merge_requests/123
If we want to add other attributes props (one by region), like Florent is currently doing with UI Suite Bootstrap, let's do that in a separate issue.
- 🇫🇷France just_like_good_vibes PARIS
just_like_good_vibes → changed the visibility of the branch 3485201-1.1.0-from-layout to hidden.
-
just_like_good_vibes →
committed 1cb2625c on 1.1.x authored by
pdureau →
Issue #3485201 by just_like_good_vibes, pdureau: From Layout Options to...
-
just_like_good_vibes →
committed 1cb2625c on 1.1.x authored by
pdureau →