🇷🇸Serbia @finnsky

As far as I know, the rest of the SDC cores are one in Olivero and several in the Navigation module. So yes, most of the SDCs are here

#71 Yeah!

This really does look better!

And if variants are just property bundles then they should work as bundles
And slots and props remain basic and simple.

This code really shouldn't be in components

{% set variants = variant|split('__')|map(v => v|replace({(v): 'btn-' ~ v})|replace({'_': '-'})) %}

@bronismateusz

Hello!

on the screenshot I see Gin theme. The error repeats in Claro?
Gin has own JS as far as i understand https://git.drupalcode.org/project/gin/-/blob/4.0.x/js/navigation/naviga...

I can't say much about the code, but I'll speak conceptually.

I like SDC for the same reason that I once loved BEM, namely:
Simplicity and sufficient universality in describing everything.

That is:
BEM - Block + Element + Modifier
SDC - Component + Slot + Prop

Simplicity and universality are exactly what the standard needs. And if we continue the analogy with BEM, then we have

BEiM - Block Element Modifier and Improved Modifier (Renderer)

Which is not scary in general, but complicates the simplicity and elegance of the standard.

I understand that many people want this and even understand their need, so in general I am not against it.
But the only thing I would like to do is to leave these variants and other things outside the main SDC properties (props/slots)

1. Components can work without mentioning Variants
2. Variants are part of the standard, but they are not required to be used. The documentation may contain lines about the optionality of variants. For example, add Variants when you need a description of properties or a different render

And so I like where we are going with these components! Thanks everyone!

Now CR:
Impacts:
Module developers
Themers
Distribution developers

I will merge it, if noone disagree :)

Thanks for participating everyone!

I added one small change by removing the useless wrapper.

- In comment 50 I see screenshots from the old toolbar. While we need to test the new one. This may be my mistake. When I restarted Tugboat I did not enable our module in it. Sorry

- In the attached Google doc I see sentences like

Complementary renamed to: administrative sidebar complementary

while I see that it is pronounced in VoiceOver as
`Administrative sidebar, complementary` so nothing needs to be renamed?

What is the desired level of granularity using the example of the navigational sidebar?

We don't know. Let's choose the best option

Other questions in the doc seem to me to be beyond the scope of the current task. At least we can definitely solve them later and not delay this last release blocker.

So I'm moving the task to review again and asking you to do the following:

Answer the question.
Does the current MR satisfy the minimum accessibility requirements? And if it doesn't, what should we do to achieve them?

finnsky → created an issue.

You can work with SDC not only in Drupal

Hello!

I see unrelated changes. That gitlab-ci.yaml file.

Hello all!

I see unrelated changes in MR, gitlab-ci.yaml

Hello!

I see unrelated changes here. gitlab-ci.yaml
Can we avoid it?

Hello all!
Thank you for work here!
Could you please transform patch to MR? It will be easier for review

Hi everyone!
I kind of forgot about this topic!
Let's continue, the MR looks good but contains unrelated changes. In particular .gitlab-ci.yml
Why is it here?

Here's what we found with Github Copilot:
They are in CSS files

bgblue — refer to a blue background (background blue).
bgred — refer to a red background (background red).
clearfix — commonly used in CSS to fix float issues.
closethick — relate to the thickness of a border or element.
minusthick — relate to reducing thickness.
plusthick — relate to increasing thickness.
zoomin — relate to a zoom-in effect.
zoomout — relate to a zoom-out effect.
tabledrag — relate to tables and their styles.
tableselect — relate to selecting rows in tables.
tablesort — relate to sorting tables.
transferthick — relate to the thickness of a border or element.
twocol — relate to a two-column layout.
threecol — relate to a three-column layout.
strikethrough — relate to text with a strikethrough.
touchevents — relate to handling touch events.
titlebar — relate to headers of windows or blocks.
twistie — relate to collapsible/expandable UI elements.
extrasmall — relate to element sizes.
extraspace — relate to additional spacing between elements.

Is this core and/or contrib?

It doesn't really matter
For now, this is part of the experimental Navigation module. But further on, I have reason to believe that this will become global styles of the Drupal admin theme.
And I really don't like the presence of

/* cspell:ignore wght */

in CSS files, because

This is a direct alternative to font weight, that is, it will be used in almost any file.
This is a completely valid and documented CSS unit.

The CSS core developer does not know that the dictionary is unfinished and, frankly, should not know.
Checking the code against an unfinished dictionary and causing pipeline crashes in this case is another obstacle for novice developers, which simply should not exist.

Therefore, I propose either

1. Merge a new version of the merge request
Leaving at least
ital - we already know that this is OK
wght - for me, this is no different from dflt which are already in the dictionary

2. (Globally) Remove the requirement to check the dictionary. Leaving only notifications for the core committer that some words in the merge request do not match the dictionary. Thus, the decision that this is a spelling error or a dictionary flaw (as in my case) will be made on the spot before the commit

I don't know.

I don't have much time for contributing right now.

I thought that tickets like this are a normal way to treat these pipeline crashes.

Now I just want to understand why a normal css unit causes pipeline crashes.
Why should we add comments ignoring a normal css unit?
It's not a weird word but a documented property

@quietone

I just ran through the existing words and I see an inconsistency in your comment

The dictionary has:

autop - possibly a misspelling of autopilot.
dflt - possibly a misspelling of default.
devel - possibly a misspelling of development.
delsp - possibly a misspelling of delete space.
denormalizable - technical term, but could be perceived as a misspelling.
dotenv - technical term, but could be a misspelling in other contexts.
dnumber - possibly a misspelling of number.
widthx - possibly a misspelling of width.
writeln - possibly a misspelling of write line.
xmlhttp - possibly a misspelling of XML HTTP.
yamls - possibly a misspelling of YAML (no plural).
zoomin - possibly a misspelling of zoom in.
zoomout - this may be a misspelling of zoom out.

But the most important question.
Why do pipelines crash when I write valid CSS?
Why the dictionary?
Shouldn't it just be optional in that case?

at least this needs to be added
https://www.npmjs.com/package/@cspell/dict-css

@anjali rathod Thank you for joining Navigation!

You don't need to assign tasks to yourself. Usually it's enough to write that you're working on it.

https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

I might need to move what I did into a separate task.

Yes, it could be a good replacement but it seems that the library I suggested is no longer supported.
https://github.com/taye/interact.js

Also not clear to me how this interacts (or not) with (dialog)

It is related in such a way that we also have to replace or remove
https://jqueryui.com/resizable/
Here is an option with replace

Rebased. Please review

Everything looks much cleaner and simpler now!
Please take a look

finnsky → made their first commit to this issue’s fork.

Then perhaps it's worth considering why the dictionary throws exceptions for fairly popular properties that are fully supported by the core? I don't understand the purpose of the dictionary in this case

These properties are obviously not drupalisms.

I don't think so, the drupal dictionary contains drupal specific things. While I want to add global CSS properties.

Now another thing cutted
https://gyazo.com/8144ef07472d3a461ab478946704ba81

I added quick implementation of
https://web.dev/articles/declarative-shadow-dom
Almost current Drupal browserslist https://caniuse.com/declarative-shadow-dom

Works as a charm. Toolbar button fixed ;)

finnsky → made their first commit to this issue’s fork.

it affects popover globally.
https://gyazo.com/2ba673f49f6a35e1040bac2059a43f06

Please check css linter.

Yes, this is a more accurate way of determining child nodes. + 1 to RTBC
Thank you!

Fixed in related issue.
https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work

We need to apply this here for items which does not have icons.
Should be enough imo.

https://dev.to/whitep4nth3r/hide-text-in-css-pseudo-elements-from-screen...

Thank you all for participating!

Great! Thank you!

I just would like to keep properties names in title.

I can't say that I agree with the wording.

Yes, these properties are used only by navigation so far. But even without navigation, it makes sense to add them since this progressive technology can be used by others.

finnsky → created an issue. See original summary → .

Thank you for review. Gonna check now.

finnsky → created an issue.

I rolled back the use of icons to the clean class.
Now everything works. And that problem from the video is no longer repeated.

Let's test!

This video explains a lot ;)

I'll check it

Thank you for work here.

I would like to ask for some improvements.

1. Use core lint-fix yarn task.
2. Try to avoid testing in css
3. Please avoid !important
4. Please check if it possible to fix outside of module components. It is not strict related with toolbar-menu component. So why that changes here?

I want to thank everyone again for their enthusiasm!

But also to clarify that this MR already makes many useful improvements!

Let's do everything to get it merged as soon as possible and continue discussing the icon configuration in a separate issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/11172
For example, there are several comments here that we should address here, or do later?

Thank you!

The "Edit" button is read aloud as "Ed Edit" to screen readers

It should gone with this MR landed:
https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work

We should focus on the Icon API here, and not see any visual regressions from the state of 11.x

We should not compare the appearance with version 11. But with the design that does not exist.

In fact, only the color and font thickness of the button are beyond the scope of this task. But this button is not in the design. What is currently in version 11 is something random. In addition, it was added incorrectly.

So I don't see anything critical if it is here.

Thank you for starting review it.

2. The Top Bar edit button is regressed as well. It has the wrong icon now and the wrong font-weight.

They are actually structural fixes. Regressions were added previously.
Few of them out of scope of that ticket. But they should be done anyway.
https://www.drupal.org/project/drupal/issues/3505291 🐛 Undo some changes Active

1. I prefer not to duplicate class functionality. We already have [class*="toolbar-button--icon"]
We should use it. Just add a check for the text attribute.

2. In #6 I pointed out that the close button on mobile is also broken.

Well then let's leave it as is for now.

This MR already solves many problems on the frontend.
In the modules that use Navigation nothing will break and it will simply display the first two letters and the Icon can be added in CSS.
Which is quite flexible.

What is really needed here is good testing.

CSS option for extending still exists. But maybe we can make it ideal ;)

@mogtofu33, @pdureau

I have question, maybe you have idea?

How technically modules which extend Navigation may set their own icons with current approach?

i'm thinking about
modulename.icons.yaml
and
{{ icon(‘modulename’, icon, { class: ‘toolbar-button__icon’, size: 20 }) }}

It can be SDC option:
icons_lib:
default: navigation

Modules like: (before they had CSS option for this)

https://www.drupal.org/project/navigation_extra_tools →

I added a role and text for the button. Nw tests passed.

But a11y needs to be checked here.

And in general, what is top_bar? What is its role? Header or navigation?

It is not small hot fix but reason is in wrong icons management in module.

https://gyazo.com/cfbb9da933c5d883bf840b02a9b6a522

All buttons with only one icon broken. Close on mobile as well.
I would like to fix that system problem in global linked ticket.

finnsky → made their first commit to this issue’s fork.

+++ for initiative! that CSS logic is much better than original one. Better to check presence of elements and not :empty()

https://www.drupal.org/project/drupal/issues/3476471 📌 Replace js message with webcomponents Active
Merged now. So it can be used as testing area for `template` and Shadow Dom approach

Only one test failure:

2x smaller StylesheetBytes!!!

All tests green!

I used the power of CSS

We couldn't do this before because we didn't have an element in DOM. But now we know that if there is no icon we will add an attribute. I think it's great

All that's left is to fix the tests. Yaay

I have idea :)

yeah, seems so we need to add checking if icon in existing icons list. this should be condition.

Is there something we need to change in the Icon API side?

Seems no. Looking good! Thank you!

I looked at the MP! It looks great!

Here's what I added;

Since the icons have the same namespace, they can't be in different folders to avoid name collisions (spoiler: there was one already. Chevron in the button and menu item)
I moved all the icons to one folder

We can't use the toolbar-button--icon class for all the icons. Firstly, it doesn't follow the Bem style, and secondly, we need different classes. I added my own classes to the icons right in the config! Great!!!!

I removed the fill attribute from the icon. It's much better to set this from the CSS, especially since we have classes

Added icons to places where they weren't there yet. Different chevrons. Back button.

Corrected the styles and removed some hotfixes from recent tasks

Corrected the ajax animation

Corrected the messages

What needs to be done?

Need to fix tests

And most importantly:

If there is no icon, we should display the first two letters of the link name
https://gyazo.com/ed31b0f04e1d5ef87e278152bf1a98f1

In toolbar-button.twig
{% if icon is empty %} does not work
{% if icon|render is empty %} works

To check, add any link to the top level of the menu

https://www.drupal.org/project/drupal/issues/3506880 🐛 Nightwatch test Expand/Collapse passed with broken Navigation Active

finnsky → created an issue.

Then I would solve the testing problem in the next task. Here we have a critical bug.

I have no idea how the BigPipe works and how well it is handled by Nightwatch. But now the Navigation module does not work at all in Olivero and partly in Claro.

@smustgrave i'm not sure. what you mean? tests green. we need review here

Great news! Thank you!

We need to pause icons tickets and concentrate on Icon API implementation

https://www.drupal.org/project/drupal/issues/3483209 📌 Navigation leverage icon core API Needs work

Done!

Messages appear in Umami demo profile only for now.
https://www.drupal.org/project/drupal/issues/3441100#comment-15984062 📌 Integrate Umami message Active

Hello! Thank you for working on it.

Could you please create MR here as described in https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →
It will be easier to review and test.

Thank you very much!
This is great step!

We also need to clean up some css tricks which we did for iconless buttons. I'll care about it in next days.

@mogtofu33 hello!
It is possible to update that MR? Would be cool to have it onboard before stable release

Pushed some css which make work done.

I would like to have some js optimisations here aswell

finnsky → changed the visibility of the branch 3505291-undo-some-changes to hidden.

finnsky → created an issue.

finnsky → changed the visibility of the branch 3365377-metadata to active.

Rebased. Thank you Please review.

I suddenly discovered that we don't need context and behavior.

One backend fix need to be applied here. We can go with here.

I was saying that these headings were there from the very beginning of the project and I don't think anyone ever seriously looked into the semantics or accessibility of the headings. That's why we removed them.

What we really need is a quality review of accessibility and structure. And depending on the review results, add or remove headings. I'm not sure they are needed at all.

@ckrina wrote in #22

Removing the "prove the need" because it's proven already that this is necessary to fix #3438976: [PP-2] Implement a caching strategy for the menu links. Discussing this with @plopesc.

But I don't understand how caching is related to accessibility and semantics

Prove the need

was in the title not because of caching but because of a11y.

@vladimiraus thank you for care about this issue ;)

@smustgrave here usual random failures. all green now.

Follow up issue
https://www.drupal.org/project/drupal/issues/3504265 🐛 Yarn watch task broken Active