- Issue created by @pdureau
Hey Pierre, thanks for a thorough check on the components and sorry for a late reply, I've been quite busy but I'll start cleaning this up soon and keep you posted on that
- Status changed to Needs review
9 months ago 12:00pm 15 February 2024 Hi Pierre, so far I fixed/improved the following:
- Heading component
- Nav-item component
- Accordion component
- Table component
- Array of HTML classes
Regarding
title_prefix
&title_suffix
, these variables are not slots in the sense used by Drupal's SDC schema, but they serve a similar purpose in terms of allowing customization of the page title, for Radix design philosophy, I treat twig blocks as slots and not any other variablesI'd appreciate it if you check these changes out while I work on Toasts, thanks
- Assigned to pdureau
- 🇫🇷France pdureau Paris
Heading component
https://git.drupalcode.org/project/radix/-/tree/6.0.x/components/heading
Unused attributes variable
heading_attributes
is used instead of the defaultattributes
variable.It is defined in the YML (like badge component do for
badge_attributes
, or page-footer do forfooter_attributes
) but it would be better to use the defaultattributes
variable.And it will prevent this kind of verbose precautions:
{% set heading_attributes = heading_attributes ?: create_attribute() %}
Duality slots / props
So, now we have
heading_content
slot as a Twig block ANDtitle_link
&content
props inside the exact same block:{% block heading_content %} {% if title_link %} <a href="{{ title_link }}">{{ content }}</a> {% else %} {{ content }} {% endif %} {% endblock %}
What will happen if we call the component with only the slot ?
"#type" => "component", "#component" =>"radix:heading", "#slots" => [ "content" => ["#plain_text" => "Lorem ipusm"], ]
Only the props ?
"#type" => "component", "#component" =>"radix:heading", "#props" => [ "title_link" => "Foo", "content" => "Bar", ]
Both slot and props ?
Let's be careful with the Twig block system. It is useful for template inheritance, but in component driven development we don't do template inheritance.
nav-item component
https://git.drupalcode.org/project/radix/-/tree/6.0.x/components/nav-item
Unused attributes variable
nav_item_attributes
is used in template but not defined anywhere.
But as seen for heading component, instead of adding the prop in the definition, it is better to use the defaultattributes
variable.link slot
The template looks now OK:
{{ link|add_class('nav-link', is_active ? 'active' : '') }}
But not the definition yet. It went from undefined object to undefined array.
Accordion component
Not reviewed yet. I will update this comment later.
Table component
table
prop is now split in 5 slots (table_caption, table_colgroup, table_header, table_body & table_footer) and 5 props:- caption: String
- colgroups: Undefined array
- header: Undefined array
- rows: Undefined array
- footer: String
Like for heading component, what to do with duality slots/props ?
Array of HTML classes
Perfect :)
title_prefix & title_suffix
Indeed, they are not always slots. Fro example,
{{ title_suffix.contextual_links }}
in media component.
But they need to be defined anywhere. What is expected inside ? - Issue was unassigned.
- Status changed to Needs work
9 months ago 4:05pm 20 February 2024 - 🇫🇷France pdureau Paris
Review done. I would be happy to continue this conversation if you still need me.
Pierre great work as always, by any chance would you like to be a co-maintainer? makes rather more sense that way and you can just start contributing if you have free time in between all your other projects
- 🇫🇷France pdureau Paris
Hi Sohail,
I would be honoured to become maintainer of Radix, but I am already maintaining a Bootstrap5 theme: https://www.drupal.org/project/ui_suite_bootstrap →
Sharing feedbacks about SDC integration is easy for me, because it is very operational and, most of the time, simple binary decision (it is OK ? is it not?). However, becoming more involved and part of the team, I may feel uncomfortable dealing with such similar projects.
yeah I see your point, and I need to give your theme a go, thanks a lot anyways but feel free whenever you wanted to jump in, let me know.
Otherwise I appreciate your contribution in general- 🇫🇷France pdureau Paris
and I need to give your theme a go
Great :)
If you are interested about our SDC implementation (which will become default this summer I hope, replacing current UI Patterns 1.x → components), we have a specific branch with them: https://git.drupalcode.org/issue/ui_suite_bootstrap-3412076/-/tree/34120...
Except
table
component which is under rework, there are pretty good IMHO.links
,variants
andstories
properties of components definitions are UI Patterns specific. - Status changed to Fixed
8 months ago 1:01pm 7 April 2024 Thanks a lot for the help Pierre and a lot of good tips and hints, most are fixed now, some are opinionated but all is good so far. Feel free to re-open with your great suggestions, marking this as fixed for now
Automatically closed - issue fixed for 2 weeks with no activity.