🇷🇸Serbia @finnsky

Now seems all works as expected.
Please review.

Some issues with RTL, but it close to finish now.

Hidden outdated branches.

finnsky → changed the visibility of the branch 3197758-create-a-new to hidden.

finnsky → changed the visibility of the branch toggletip-11x to hidden.

Looks good to me, checked on all screen sizes and rem definitions.

Fixed 2 small bugs

1. scope of mobile expand button. now it is control-bar instead of top-bar
2. duplication of --admin-toolbar-top-bar-height variable in control-bar

That small bugs so i believe they can be applied as RTBC.

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

CR: https://www.drupal.org/node/3454916 →

I think it may need change record since we add new function in Drupal Twig here.

@m4olivei

I love that idea. existing twig function isn't really flexible.

This ticket not only admin-toolbar__tooltip. But about all possible hidden elements which are force displayed by layout builder script.
I believe logic should be different here. Please take a look at the Proposed resolution

1. On `hide content preview` add checking. and hide only visible elements + Add special attribute `data-hidden-by-layout-builder` to them.
2. On `show content preview` show only that ^ elements + remove that attribute

Sorry, keep in in rtbc. we also had that heading ID in footer.

Follow up https://www.drupal.org/project/drupal/issues/3452724 ✨ back the ID and aria-labelled-by with JS Postponed

finnsky → created an issue.

mikelutz → credited finnsky → .

finnsky → changed the visibility of the branch 3450498-navigation-tooltip-appears to hidden.

Actually it is not our false. Layout Builder forced show all items on `enable content preview` checkbox.

Gonna cehck it deeper. seems that tooltip uses own class. not sure why

I've added small comment.

Looks good to me. Also i've removed outdated classnames. they are only should be on block level now.

Thank you for work here.

I would better resolve it in main issue about no-js behaviour:

https://www.drupal.org/project/drupal/issues/3396174 🐛 The toolbar should be usable without JS Needs work

Thank you for review. This was fast approach.

finnsky → changed the visibility of the branch 3443577-navigation-overlay-z-index to hidden.

Thank you! Exactly!

I've added very quick List component as SDC for now.

https://github.com/iberdinsky-skilld/ui_suite_material

Probably concept of scoped css variables is out of Novice expertise area.

That changes looks good to me! Thank you!

Yes, also it can extends type with custom widgets or fields.
https://rjsf-team.github.io/react-jsonschema-form/docs/advanced-customiz...

What can be useful for example in Attributes field.

finnsky → created an issue.

Works for me after
$settings['extension_discovery_scan_tests'] = TRUE;
was configured in settings.php

finnsky → created an issue.

Problem that:
1. We cannot add test coverage here. Only cover attribute presence. But it is 1% of feature. Probably it is possible to do with Nightwatch. Probably...
2. We already have that type of attributes without any test coverage. Tests shouldn't block it.
3. This is temporary solution until we not have icons library.

@smustgrave,
this is Nightwatch issue: https://www.drupal.org/project/drupal/issues/3393400 ✨ Evaluate adding Nightwatch tests Active

But this issue shouldn't be blocked by Nightwatch setup.
We had that type of attributes before.

I'm not even sure this feature can be tested by Nightwatch.
Tricky CSS there, until we don't have Library of icons in module.

We need to add type="button" for all UI buttons here. Close, collapse/expand etc

Let's start when parent feature will be merged.

Let's fix CSS here:

https://www.drupal.org/project/drupal/issues/3450103 ✨ .admin-toolbar__footer CSS fix. [follow up] Active

finnsky → created an issue.

I think that CSS can be simplified. But i don't want to block this feature.

I would like to fix some css and twig here.

1. Imo only Nightwatch tests can cover it. So we need to write test for this after Nightwatch setup will be merged.
2.

However, we cannot work on those possible conflicts until one of them is merged

We can work on both in same time.

Gonna review linked issue aswell

For test add any 2nd level menu link to Administration menu:

I tried to solve this problem differently.

At the moment, the button--has-icon class does not mean that the variable is actually set, all we have is the presence of the --icon variable

I tried to combine
https://css-tricks.com/logical-operations-with-css-variables/
and a double gradient depending on the presence of the variable and everything seems to work well.

Please review!

+1

https://github.com/prettier/stylelint-prettier

in core now. https://www.drupal.org/project/drupal/issues/3409048 🐛 Configure postcss formatting after stylelint and stylelint-config-standard update Needs review

+1
Thank you!

I don't think proof of concept should be rebased. But thank you anyway.

Thank you for backporting this.

Some notes.

Since that original script was written lot of things were changed.
We need to rework at and adapt to current logic.

1. I see shadow on mobile but not on desktop
2. I see broken header and wrong padding relative to footer.
https://gyazo.com/f08ad11ee5f5d21f4290af428d220d72

3. I see some flickering on popover open
https://gyazo.com/b80215700f117abb7350000800758d08
probably not related to this fix but better to check

I like CSS-only positioning. But what about

1. my/at/flip in https://api.jqueryui.com/position/ ?
2. also not clean how to set maxWidth etc.

Floating UI seems allows to keep all previous features.

Thank you for fix. But it is duplicated issue. We need another method in fix

https://www.drupal.org/project/drupal/issues/3443577 🐛 Navigation overlay z-index is not defined Needs work

Looking good to me +1 rtbc. I've added 2 small improvements.

1. Added type=submit to expand/close buttons to avoid LB form submit.
2. Added minor css fixes. Font size for LB text and always wide sidebar in css.

finnsky → changed the visibility of the branch 3444699-navigation-layoutbuilder-ui to active.

finnsky → changed the visibility of the branch 3444699-navigation-layoutbuilder-ui to hidden.

We had discussion about option to extend components
https://drupal.slack.com/archives/C4EDNHFGS/p1715090009402889

finnsky → changed the visibility of the branch 3446855-get-rid-of to hidden.

Cool. Gonna continue then.

finnsky → created an issue.

https://www.drupal.org/project/drupal/issues/3446818 ✨ Replace dialog positioning with floating-ui Active

finnsky → changed the visibility of the branch 3446818-dialog-posit to hidden.

finnsky → changed the visibility of the branch 3446818-dialog-posit to active.

finnsky → changed the visibility of the branch 3446818-replace-dialog-position to hidden.

finnsky → changed the visibility of the branch 3446818-dialog-posit to hidden.

finnsky → created an issue.

Perhaps SDC will be useful here

I've added an icon component and now using SDC I replaced the icon to umami. We need to decide how this yaml config also can be overriden. But globally i prefer this obvious and simplest way.

This fix breaks collapsed sidebar expand popover functional.

I think it is good place for
1. SDC.
Before SDC was stable we had some efforts in that direction https://git.drupalcode.org/project/navigation/-/merge_requests/72/diffs

We need to convert existing components and add new components as sdc.

2. Storybook. I still believe it is better to check all theories until they adapted on Drupal.

Some previous SB versions.
https://storied-pixie-3d54e7.netlify.app/iframe.html?args=&id=page-examp...
https://sparkling-gaufre-c402f2.netlify.app/iframe.html?args=&id=page--b...
https://thunderous-treacle-600c5c.netlify.app/iframe.html?args=&id=page-...

I think we can merge it and create META for other tests.

I've removed initial titles.
Now they are not defined.
Only block level titles we have now.

Backender review required here.

Test passed now.

I've added special tag for module for :

`yarn test:nightwatch --tag navigation`

Probably we need to add better explanations here.

Nope. I just checked for regressions in tugboat instance

Here also we need to fix this one.

I suggest to add to resize observer for that shadows.

I've tested. seems all fine.

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

@javi-er yes. i got same result with
forceFallback: true. in core/modules/layout_builder/js/layout-builder.js:166

all works fine in latest chrome and safari

without this fix failure.

probably we need to ask for help layout builder team.

finnsky → changed the visibility of the branch 3446340-fix-z-index- to hidden.

finnsky → created an issue.

Looks good. Issue fixed. No regressions. Thank you!

Seems tests need deeper rework.

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

I got what you mean, but it is out of scope of navigation module. even more. this is not new problem

problem now:

problem before:

So i suggest to open Olivero ticket for this.