🇷🇺Russia @kostyashupenko

Good catch.

It's now ready for review.
made also span element not interactable visually (no hover styles). Also added few fixes for forced-colors: active mode

In progress

Can be related

RE #10
i'm attaching correct gif with the results

btw @Gauravvvv i think it is not required to change those px values, because if you will look to the context of the selector you will understand that the styles are used to hide visually an element.

Keeping on Needs review still

MR is ready for review. In #5 i have uploaded wrong gif with the demonstration of results. Will upload correct gif later, but you can test in any way.

Results

kostyashupenko → created an issue.

I think we have to keep rems.

Themes and modules can conrol everything by overriding --admin-toolbar-rem variable, no?

nod_ → credited kostyashupenko → .

Resolved CSS linting issues. Just by running `yarn lint:css --fix`.

Issue seems fixed. Here are 4 screens illustration outline rings for hover and focus states, for both collapsed and expanded states.

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

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

Seems there is an extra setting allowing you to display as much levels in specific menu as you need.

For now closing the issue, but if you think that this behavior should be changed, feel free to re-open it

I'm gonna mark it as Needs review since i've inserted few modifications

lauriii → credited kostyashupenko → .

Please test all 3 issues and review MR

kostyashupenko → created an issue.

Pushed new commit with only fixing of the linting issues. I have tested MR and seems issue is fixed, now you can vertically scroll sub-nav bar

mmm, i can't reproduce it @rkoller

so you said:

enable automatic dark mode ticked, forced colors active and preer contrast set to more

In the system OS settings on Mac OS i have enabled dark mode
in MS Edge browser i have enabled forced-colors: active
and this preer contrast set to more i think you were talking about system OS settings again? I see range bar in the settings to increase contrast (or this setting somehow i can apply inside of MS Edge)?

kostyashupenko → created an issue.

Fixed both bugs

Description was updated

kostyashupenko → created an issue.

Task is finished. Now Olivero, Claro and Umami are not overriding hover and focus styles of the navigation module

UPD:

Seems we can't go with @layer at-rules.

@layer is useful only when need to override user agent styles seems like. But any CSS property written by us (author styles) has bigger specificity than @layer's. Always. Which means if we will wrap some code in @layer - user themes will still override navigation module styles.

Will try to increase specificity of our selectors without @layer

After investigation i found that not only Umami overrides styles of Navigation module, but also Claro and even Olivero sometimes. It's related to some global selectors which have bigger specificity than our global:

:where(.admin-toolbar) *:focus {
    outline: var(--admin-toolbar-size-focus) solid var(--admin-toolbar-color-focus);
}

Here is screenshot illustrating that styles from Claro theme are overriding :focus on logotype in navigation bar.

And now i think that having such selectors in user themes is a pretty popular thing:

a:focus {
    ... something like setting outline globally for all links ...
}

And now we can't guarantee themes will never override navigation module styles. So we have to increase specificity of our selectors. To be sure we are safe in terms of styles no matter which theme is used.

@finnsky suggested to go with this way https://developer.mozilla.org/en-US/docs/Web/CSS/@layer and i will try to implement it now

I also removed useful from my pov navigation_preprocess_breadcrumb hook which was added to add an extra link "Back to site" in breadcrumbs:

Tell me if we have to keep it or not. But from my pov it should be handled not in the scope of Navigation module

I tried to reproduce in Safari with VoiceOver enabled - i was not able to do that. But i've got the idea that probably the fix provided in this MR https://www.drupal.org/project/navigation/issues/3438039 🐛 Visual glitch while sub-menu is expanding / collapsing Needs review will resolve this problem. Let's try to re-reproduce again once side issue is fixed @rkoller

Seems the fix was simple, ready for review

I have resolved my feedback on MR, but once i have pushed new commit, we need again someone to review everything

kostyashupenko → created an issue.

mmm @ckrina i have pushed one little commit to get rid of `toolbar-button--level-1` selector.

But i also left a comment in MR. We have so many CSS variables for fonts which will never be used seems like. Tagging to `Needs work` for discussions

kostyashupenko → created an issue.

Re #11
i think i made a mistake. Because seems on design 0-level links and buttons are equal to 1-level links and buttons in terms of text

1. Created list with font-size, line-height, font-weight and letter-spacing CSS variables.
2. Font-weight - because potentially someone will want to customize font. And we have 400, 500, 600, 700 font weights in our design system. Potentially custom font will never have some of those weights, so it will be easy to override font weights from css.
3. Letter spacings are also documented. Just to be able to customize it (coz it's pretty dangerous css property, which is working "fine" only with current font, but will not work with custom fonts)
4. Reworked tooltip a bit (now fully matching Figma)
5. Didn't touch breadcrumbs. We still have code related to breadcrumbs, but idk about the current state of this feature. I remember we did an override of breadcrumbs with some custom styles, but not sure if it's still actual feature or forgotten.

I suggest you to quickly review. In case of minor feedbacks -> let's create follow-up issue, because potentially we can get some (or lots of) conflicts.

Done for #12

@ckrina just enable core theme "Stark" and set it as default theme. Then visit homepage )

Rebased, conflicts resolved. Yarn lint:css tells that there are linting problems in toolbar-popover.pcss.css file. However i didn't touch this file in the scope of this ticket, so should be resolved in some other issues. Current MR contains only required changes by this ticket.

Kanchan Bhogade, seems you have to git pull from 1.x first. The errors says that there is not file NavigationTopBarTest.php to patch, however this file is exist in 1.x https://git.drupalcode.org/project/navigation/-/blob/1.x/tests/src/Funct...

Restoring needs review

Can we get rid of ::before at all in toolbar buttons?
And to have just

... optional 2 first letters...


With the own tag and specific classname it would be much easier to deliver 2 letters. Also 100% it will be easier to integrate 3rd party icons.
Please share your opinion

Also i see so many text variations on design. Are they all 100% used in Navigation module? Basically the scope only is sidebar + top bar. Did i miss something?

Would be nice to clarify this ticket. We don't have so granular components in Navigation module, like for instance component "Text" with 50 variations. I think this ticket is more about review of the current styles for the all texts?

Should be ok now. Works in MS Edge also

holy @ckrina. It works :O
gonna dig more & maybe complete this ticket

I think better wait for https://www.drupal.org/project/navigation/issues/3427657 📌 Implement the new designs to the drawer Needs review

Here is screenshot from 10.3.0-dev

Let's test it with drupal 11.x because related fix in Claro theme was provided only for 11.x https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/claro/...

For 10.x sticky table header doesn't work (due to missing fix in Claro).

By default disabled

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

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

Actually let's have it under Active status. Since some work can be already done

It's better wait for https://www.drupal.org/project/navigation/issues/3427659 📌 Implement drawer functionality Needs review first

Let's wait for https://www.drupal.org/project/navigation/issues/3427659 📌 Implement drawer functionality Needs review first, because MR out there contains lots of new colors which are hardcoded. Once task is fixed -> you can re-open this ticket

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

Postponed because this functionality is already partially done here https://www.drupal.org/project/navigation/issues/3427659 📌 Implement drawer functionality Needs review

Gonna try it

Another feedback is about uploaded file. Instead of filename "logo.svg" we'd better have preview?
And btw just got cool idea - is that for preview we can use the same box 32x32 pixels for the user so before saving the form he can already check how logotype will look in such small box.

We also have to create new image style 32x32 with Scale effect (no upscale). Scale effect because we can't crop logo, and no upscale because we have static box 32x32 pixels. So logotype in this box with such image style effect will be always centered vertically and horizontally, which is ok.

But current text "Recommended image dimension 32 x 32 pixels" is not so meaningful. Because actually we don't care. Proportions of logotype can be any. We have to say to the user that logotype will be resized proportially without cropping and it will be displayed in 32x32 box and centered. Something like this, but shorter ;)

There are still several existing data() methods:

% yarn lint:core-js-passing

core/misc/ajax.js
  1104:17  error  Prefer WeakMap to $.data  jquery/no-data
  1111:31  error  Prefer WeakMap to $.data  jquery/no-data
  1580:7   error  Prefer WeakMap to $.data  jquery/no-data

core/misc/autocomplete.js
  224:13  error  Prefer WeakMap to $.data  jquery/no-data

core/misc/dialog/dialog.ajax.js
   59:31  error  Prefer WeakMap to $.data  jquery/no-data
   60:13  error  Prefer WeakMap to $.data  jquery/no-data
  105:24  error  Prefer WeakMap to $.data  jquery/no-data

core/misc/tabbingmanager.js
  297:25  error  Prefer WeakMap to $.data        jquery/no-data
  302:9   error  Prefer WeakMap to $.data        jquery/no-data
  314:25  error  Prefer WeakMap to $.data        jquery/no-data
  332:13  error  Prefer WeakMap to $.removeData  jquery/no-data
  340:13  error  Prefer WeakMap to $.data        jquery/no-data

✖ 12 problems (12 errors, 0 warnings)

1. regarding ajax.js and 3 remaining errors - i think all 3 errors can be fixed, but need to research (i just don't have available time anymore for this, so skipped)

2. regarding autocomplete and dialog - there are some data methods which are still exist, which are related to the plugins itself (jquery autocomplete, jquery dialog). I didn't touch those 2 files at all, i think the better way is to wait for replacement of jquery autocomplete in core, and replacement of jquery dialog to native dialog element functionality.

3. regarding tabbingmanager - i didn't understand how to reproduce it in Drupal for making local tests. From what i saw in tabbingmanager.js - this file is fixable in terms of replacing jquery data() method

4. Also i prefer to use getAttribute() instead of dataset, coz getAttribute works seems at least 2x faster https://www.measurethat.net/Benchmarks/Show/14432/0/getattribute-vs-dataset

Still on it

Gonna kill this task

I think it's duplicate of https://www.drupal.org/project/navigation/issues/3432203 🐛 With the navigation menu collapsed the shortcuts submenu is auto expanded when a bookmarked page is accessed and hiding top level menu items Active

@Tirupati_Singh The displace functional described in core here https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/displace... so no matter what we are building - module or theme, we should respect core displace functionality.

This task have 2 problems:
1. Table header in Claro theme (see https://www.drupal.org/project/drupal/issues/3432298 🐛 Sticky table header is not sticky if --drupal-displace-offset-top is not defined Fixed and btw this task is already fixed)
2. "data-offset-top" html attribute in Navigation module - was added always, but instead should be added when we are really have top bar rendered (this is what was fixed in 200 mr)

Yes but resolving table problems should not be in the scope of navigation module

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

I was thinking how to bring minimal effort for solving this idea and here it is:
1. We have to redo CSS logic, we need to get rid of ".toolbar-button::before" selector with mask-image and replace it by embedded svg image in twig template with source(), like:

// toolbar-button.html.twig

<button { ...props }>
  <span class="toolbar-button__icon">
    {{ source('path/to/root/assets/icons/folder/' ~ iconname, true) }} 
  </span>
</button>

btw, second param in source() twig function true required to prevent WSOD if template defined in first param is not found.

2. We put all the icons we have currently in assets/icons folder where filename of svg file = twig's iconname. So this is a strict rule, filename of icon matters.

3. In admin back office for admin menu links we can put some select field (or fieldset with the group of radio inputs). Each element = 1 icon. Preview of the icon + its label (based on icon filename). So user can browse all available icons at once and choose any icon he want.

4. Now how to use external icons? I'm not backender, but seems like we can create some folder in modules or themes, named "navigation-icons" where all custom svg icons can be stored. And then Drupal can scan all of the icons from specific folders and grab them and merge with the default icons stored in navigation module, so user in step 3 can browse all available icons at once.

Any opinions?

I have used new MR 6957, because 1251 have huge rebase with conflicts (i decided to not waste the time - that's only one reason).
Basically in my commit i was trying to resolve #17 📌 Refactor (if feasible) use of jquery parseHTML function to use vanillaJS Needs work

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

I this this task is related to this https://www.drupal.org/project/navigation/issues/3432173 📌 Decide strategy to customize or provide 1st level menu items' icons Active where we need to define one global strategy how we can deliver own / custom icons.

1. background (mask)
2. iconic font
3. embed svg <- this is a winner by the way

And i think the initial approach with ::before pseudoselector directly on button itself (.toolbar-button) was a bit wrong. Since such kind of things (which should work with own icons, external icons and even 2 letters if no icon defined) requires own wrapper, like .

Closed since fixed here https://www.drupal.org/project/navigation/issues/3423020 🐛 Flickering when loading the page Needs review

Done

gonna fix it now

Confirmed described problem is really exist.

Because here https://git.drupalcode.org/project/navigation/-/blob/1.x/templates/top-b... we have "data-offset-top" attribute added no matter if top bar is rendering local_tasks or not.

And no matter if this block is hidden by some CSS rules or not -> core's displace() is still measuring height of this block https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/displace... and creating --drupal-displace-offset-top CSS property in style attribute of html tag. That's the reason why table header have top offset.

This issue requires 2 fixes:
1. first fix is already provided in MR - now we are not rendering "data-offset-top" attribute if no local_tasks in render.
2. and another fix should be done in core. I have created new ticket in core for Claro theme https://www.drupal.org/project/drupal/issues/3432298 🐛 Sticky table header is not sticky if --drupal-displace-offset-top is not defined Fixed which i have fixed already . Reason is simple - once i have kicked "data-offset-top" attribute - there is no anymore css property "--drupal-displace-offset-top" defined, which breaks stickiness of table header (read description of core task i have created to understand better)

kostyashupenko → created an issue.

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

ckrina → credited kostyashupenko → .