🇷🇸Serbia @finnsky

As discussed in Slack
https://drupal.slack.com/archives/C7AB68LJV/p1763900832081709

removed navigation__message theme hook and added CR
https://www.drupal.org/node/3559492 →

I think this misunderstanding arose because the hooks were previously reworked, causing a merge conflict.

So I'm sending it back to review because the hooks aren't currently being changed in the MR.

Congrats to everyone!

Probably also floating-ui lib should not be internal now?
https://git.drupalcode.org/project/drupal/-/blob/HEAD/core/core.librarie...

it can be used by other module themes

Usage candidates:
https://www.drupal.org/node/3197758 →
https://www.drupal.org/project/dialog_native →

Rebased and fixed tests.

Please review

Fixed! Please review!

I've removed the check for this text.

The tests are green!

Please take a look!

Well, I'd say that in that file you're still working with a jQuery instance.

Here, pure vanilla does the same thing. And when the time comes, we can simply replace the classes and get a native dialog.

All works same as with jquery UI
https://github.com/jquery/jquery-ui/blob/7571b739fbe227a9f47c1bbbd1d9f18...

but without :)

please review.

The test failure here is due to the Shortcuts block being displayed even if it doesn't contain any links.

Accordingly, a hidden Shortcuts heading appears on the page.

The test checks to ensure that such text doesn't exist.

Perhaps we should simply remove the check for this text now.

And add a condition to display the block if it doesn't contain any links in the new task.

In any case, this isn't an accessibility bug and isn't related to headings.

I see main issue that offcanvas JS
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/dialog/o...

not allows to set multiple top displacers.
https://gyazo.com/d598a5fdfad8d8ac8b6a2e415ec7a303

So we not able to add top shift with both top dialog and top bar.

I think it's correct now.

- I reverted to using a variable.
- Removed the negative margin for the first block. It was incorrect (10px instead of 12px), and it turns out it's unnecessary now.

This means we always have 16px between the logo and the buttons.

I think there shouldn't be any difference between desktop and mobile.

@alexpott

It's not entirely clear what we should deprecate!

The SDC component and theme hook can work without Umami.
Even if Umami is disabled, many modules may want to add messages to the Navigation.

https://www.drupal.org/project/drupal/issues/3441100#comment-15770404 📌 Integrate Umami message Active

That's why we implemented the styles and functionality in the Navigation module and not in Umami.

Or did you mean something else? Please clarify!

I think it's a bad idea to use headers in different places for configuration.
It's always better to have a single entry point for configuration to avoid confusion.

Now i see

Heading elements are not in a sequentially-descending order

Probably h3 should be more logical here?

It seems we've stuck here :)

In this regard, I'm raising the previous questions and insisting on answers.

• Does the current MR meet the minimum accessibility requirements?
• And if it doesn't, what should we do to achieve them?
• Is there some irreversible accessibility error in this MR that can't be fixed later?

These headings become visible when you customize the block.

So, to me to match the issue summary I think we should probably do #99.2, which would mean changing the nav for the individual menus into div.

We've already done that :)

This MR only has one nav element.

@joelpittet Thanks for participating.

This task, in my opinion, is turning into a discussion of the approach.

Two headers or eight?

Most participants agreed with two headers.

Optional headers for each individual block seem like a problem that needs to be addressed at the core level, not here.

Last time we agreed that there is no difference between

<nav aria-labelledby="nav1">
  <h2 id="nav1">Title for navigation 1<h2>

and

<nav aria-label="Title for navigation">

Hello!

Now everything works, but in my opinion not as it should.

Previously there was a variable var(--admin-toolbar-sidebar-header); which controls the content indent. Now you have removed it. While leaving it in other places. Either remove it everywhere or leave it here.

The decision to move the content away from the header is obviously a design decision, so I can't decide anything here.

Thanks for the reply!

I'm not very advanced in Typescript. I thought this was the standard way.

I use the definitions here
https://git.drupalcode.org/project/dialog_native/-/blob/1.x/package.json
as part of the work on
https://www.drupal.org/project/dialog_native →

I think it's great when Drupal's js is typed

@berdir

yes. currently it is only one option that we have
Umami has global `a:hover` style.

What about MR I don't really like `hack` word.
`#extra-specificity-trick` probably will sounds better? :)

Seems duplication https://www.drupal.org/project/drupal/issues/3531516 🐛 Admin toolbar height is not 100% since upgrading to Drupal 11.2 Active

@nod_ That dependency not needed anymore because CSS now provided in SDC

Thank you for review!

`admin/config/user-interface/navigation-block`
This is something that will probably change. So I believe that this can also be done later.

Let's merge?

I would probably have approach review before.

Fixed @rkoller feedback from #74

@sandip hi!
thank you for work here!

Could you please close and reopen MR to restart Tugboat test instance?

finnsky → changed the visibility of the branch 3538177-remove-jquery-ui to hidden.

finnsky → created an issue. See original summary → .

Now 2 landmarks how shown on screen in #68

I also fixed the tests. Well, how did I fix them? I removed what was crashing :)

Please review!

One extra landmark found by @rkoller in Slack

Thank you for quick response!
Fixed!

Moved content to typed props.

Please review!

I don't think we need to test Olivero theme here, it is outside of that issue scope.

I think we can go into release like this!

As for the region names themselves, that's something we can decide and adjust later without rushing. Without delaying this last release blocker

Please take a look!

Finally i understood how to test it, let's reduce that amount ;)

https://gyazo.com/4c920476d2acc6ff337dfcae2c245c73

finnsky → changed the visibility of the branch 3531516-admin-toolbar-height to hidden.

1. Removed
{% if tools or context|render or actions|render %}
We have css for it. We may avoid it.

2. It is currently impossible to provide a shift of the toolbar and top bar together with displace offset top.
Therefore, let it always be aligned to the top of the screen
The drop-down popover accordingly too.

Workd good even with workspaces. Please review.

The issue seems more deeper that just height, but about behaviour of Drupal Displace globally
When workspaces enabled we have top displace calculated wrong
https://gyazo.com/52b598ad2bac75476ccc48d88e4ad98c

@mherchel

I found bugs in Tugboat:

1. on small screens: https://gyazo.com/d5ff663b2f211157fd129d687be2dd51
2. seems link toolbar-button has extra padding, at least size is different.

Let's leave this task for a quick fix, and continue experimenting with web components here
https://www.drupal.org/project/drupal/issues/3534298 ✨ Web component toolbar button Active

finnsky → created an issue.

Maybe it's not. :)
I'm not sure right now.

The top bar was added in a hurry and we need to review how it works with displace

I don't get it.
Nightwatch task fails at the end for a strange reason.

Hello!
Thank you for report!

Please provide more details.
Just tested in 1.x and 11.x Drupal
https://gyazo.com/52a901cd6c0ee0892da1665060df73d6

Pretty stupid test failure.
We check

[data-once="admin-toolbar-document-triggers-listener"]

even though we should

[data-once*="admin-toolbar-document-triggers-listener"]

because in this task data-once has two values

Please review!

Thank you, but seems not a quick fix here.

1. Position of submenu without top bar

2. Also with top bar submenu should be aligned with top of page imo

smustgrave → credited 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.