🇷🇸Serbia @finnsky

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

finnsky → changed the visibility of the branch 3504265-yarn-watch-task to hidden.

Updated according https://github.com/paulmillr/chokidar?tab=readme-ov-file#upgrading

added usePolling: true because on macos i had lot of errors like:

yarn run watch:css
node:internal/fs/watchers:207
      const error = new UVException({
                    ^

Error: EMFILE: too many open files, watch
    at FSWatcher._handle.onchange 

finnsky → created an issue.

we upgrading node packages to major versions without reading changelog ;)
https://github.com/paulmillr/chokidar?tab=readme-ov-file#upgrading

it is broken now core/scripts/css/postcss-watch.js

const fileMatch = './**/*.pcss.css';
// Ignore everything in node_modules
const watcher = chokidar.watch(fileMatch, {
  ignoreInitial: true,
  ignored: './node_modules/**'
});

so for 1.5 month `yarn watch` not working ;)

finnsky → changed the visibility of the branch 3476471-replace-js-and to active.

finnsky → changed the visibility of the branch 3476471-replace-js-and to hidden.

Thank you for review.

I added a disclaimer. Not sure what form it should take. But I tried to be formal and bureaucratic :)

Please take a look!

Perhaps we should have done it through the SDC right away. But it is quite possible to implement it through subsequent tasks.

@hetal.solanki
no need to assign tickets.
it is enough to write that you work on it.

At the moment, the module is still experimental.
We are still waiting for the final design.
We are still waiting for the icon system.

The task description says

This issue is to ONLY move the Edit button outside of the drop-down, not applying any of the other visual changes to the image above is providing.

So there is no need to make it fully consistent with the image. It is enough to just add the markup and roughly understand how it will work afterwards.

And even more so, there is no need to add deeply nested CSS that will be impossible to clean up later without significant effort

Also, there is no need to add new icons. In particular, the Pencil was already there. Let it be different, but there will not be many icons with the same meaning when we switch to the icon API. (I hope soon)
I also took the icon of three dots from https://phosphoricons.com/ where all the other icons came from.

I also think that there is no point in adding backend tests based on visual styles, for example, the icon type, but this is not my area and I did not change it.

@plopesc could you please review backend after rebase?

Gonna work on it

A few notes:

It shouldn't be a list because it's not a list.

It also breaks the output in some themes (Claro) because the item list has its own styles https://gyazo.com/4667c856e63d3d4b3459b2ab0c43b264

We should keep in mind that everything we create should (and will) be reused in the further development of the module. When we make forms and the second sidebar, for example.

And these two components: title and badge are key elements of any design system. Therefore, it is obvious to me that they have no place in the depths of the nested CSS

I remade it as I see the further development of the module components.

Result:
https://gyazo.com/cda07be66460fdec1d06fb635e99f0e2

Several fixes are needed in the tests and possibly the render array.
Please review

gonna work on it.

Rebased. Please review

It still seems to me that in this task it is much more important to understand why the script is loaded in the wrong place than to fix the script directly. This is also a bug and a much more serious bug.

@grimreaper,
Maybe you have steps to reproduce? A list of third-party modules and some other details?

Thank you for fix. Actually it is interesting why toolbar js loaded for anonymous.

Thank you. I added comment about nesting. Please check and compare with existing CSS of module.

Probably we will need to backport valuable CSS/JS changes from this MR then.
Currently we have bug with anything displaced from top.
Not only workspaces.

Also Ajax issues from #32 also global. So this prio maybe higher.

I would like to have it merged. It has some improvements about buttons and events.
On my side RTBC + 1

Fix works for me.
Thank you!

I apologize.

I read the task description poorly.

As for MR
JavaScript looks acceptable to me.
+1 RTBC

No, I mean we can search in the module history.
These links have already worked with the dropdown list. And there is already a script and styles for them.
The backend for this module has changed several times recently, so we should just re-apply it.
I think js is not needed here at all. Maybe only for optimisation.