[PLAN] Refactor CSS

As mentioned on Slack, I'd add the CSS file refactoring to separate the sidebar styles from the menu ones because in the future we might have other things that aren’t only menus there.

Also, I'd move the font definition to its own issue or child issue because:

• We have to discuss and document adding a font for the admin UI after removing it for Claro (spoiler: it'd be great to use variable fonts)
• We have to review the license of the font
• We have to plan how we integrate in: in Claro or in the sidebar/toolbar

Also, it would be good to discuss if we're going to call this sidebar, toolbar or navigation.

i see few components now. This is how we will split css/js

1. Page level. Open close aside + manage main layout shift.
2. Sidebar itself. Scrollable area + sticky area + menu exernal geometry. In JS will be scroll to active element/Resize observer etc
3. Menu styles. Links/icons/collapse/expand
4. Universal floatingUI component one is black text another is gray with arrow or something. Probably expanded as Web Component
https://codepen.io/finnsky/pen/JjwJWbJ?editors=1010

Also we need to review postcss-preset-env settings.

text-align: start;

in our current browsers stack
https://caniuse.com/mdn-css_properties_text-align_flow_relative_values_s...

Can you bring this up to date with 1.x now that [3386509] is merged?

@mherchel yep. gonna try tonight

Just added 📌 Define main components naming Active to discuss the main component naming as an outside discussion to make it easier to see which thing could be part of of the same components and which others could be separated. Plus I've added a few thoughts.

There's a lot going on here. It's really hard to review.

1. It looks like the markup is also being 100% refactored (+390. - 385). Why are we doing this?
2. We're also updating PostCSS Preset ENV here. Why? Is this keeping up with core?
3. We're refactoring checkOverflow() within the JavaScript.

Can we split this up into smaller chunks? Right now, I see the CSS which is this issue, checkOverflow, PostCSS Preset ENV, as well as all of the markup changes.

For now I'll review the CSS here, but it's really hard for me to go through all of the markup changes when there are almost 400 removals and 400 additions just with that.

Still leaving at NR so we can review the CSS.

Looking at it visually, when I switch between branches there is a difference in spacing. Gif below

Flyouts aren't working either due to the overflow clip.

Setting this to NW now because it still needs a bit of work.

@mherchel thanks for review.

Markup changed not really much but mostly related to twig formatting i did, i think i'll revert formatting fix and keep only my real changes. we will fix it in follow up. `bad formatting in navigation.html.twig` or whatever

postcss preset env can be also reverted.
I got some properties EG #5 not working. problem was in browserslist missed in package.json this change is obviously needed.

we cannot add scrollable bar and hover texts in same time. so this should be also expanded with floating UI. can be done here or in follow up.

Main problems here are still bad structure and weak BEM globally. So it is easier for me manage all together. Componentize it and cleanup. Later we can create real small followups.

Posting the agreement reached in for the BEM component name.

Hi all!

I've reworked the current build to:
- Provide simple, independent and extensible components.
- Build a flatter BEM tree.
- Get rid of unnecessary cascades.
- Provide scrolling and tooltips/popovers at the same time.
- Provide calls for closing/opening events from different places, which will be necessary for mobile styles.
- Simplify styles to support RTL
- Ensure the independence and functionality of the module in the context of any design theme. Not just the basic Claro and Olivero.
- Bring styles into line with the current version of the design.

- Current storybook: https://storied-pixie-3d54e7.netlify.app/?path=/story/page-example--basic
It looks better than on Drupal

- During the development process, I used SDC practice to create components. This does not mean that we should use this module in an experimental status. But it helped me a lot to understand the structure and properties of the components.
I didn't use SDC components like SDC components here, I just imported twig, CSS and JS.
So we can open a ticket for discussion.

- Added correct browserlist settings. They are in the core of Drupal but apparently were missed here.

- CSS variables can only be in the toolbar context.
`.admin-toolbar { --toolbar-variable }`
But I wouldn't want that. Indeed, in this case, these components become useless outside the toolbar. And I can imagine a situation where a tooltip or button can be used by developers.
So i just prefixed them
EG: `--color-focus: #26A769;` -> `--color-toolbar-focus: #26A769;`

- Collapsed toolbar elements styles controlled with
https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_container_queries
https://caniuse.com/css-container-queries

EG:

.admin-toolbar {
  container-type: inline-size;
  container-name: admin-toolbar;
}
@container admin-toolbar (width < 263px) {
  .any-child-component {
    /* styles for elements when collapsed toolbar */
  }
}

- Added variable font. The file weight is now 850Kb. At the moment we are using two weights 600 and 700. Later there may be more.
Weight is almost the same. But we can explore other formats in another ticket.

- Didn't touch breadcrumbs at all here.

- Added detectOverflow common library to add atrributes when element has overflow on X,Y axis using ResizeObserver.

Components

Navigation Button:

Element with icons and optional dropdown or Avatar.
Can be button, link or just dummy div to display in popover header.

Navigation Menu:

Controls CSS and JS on menu component. Based on Drupal menu.html.twig.
- Collapsing expanding. (+)
- Popover of child menu. (+/-)
- Set active item. (-)
- Scroll to active menu item (-)
- Collapse not active items (-)

Toolbar Block:

Simple block containing title(optionally hidden/collapsible as in figma)

Admin Toolbar:

Parent block contains slots for Logo, Scrollable content and Sticky content. Mobile background.
Js for open/close + Custom Events catching from triggers. (mobile header, mobile background, sidebar button or any button on page).

Tooltip:

Super simple component with floating UI.
Can be attached anywhere with `data-tooltip-text="Text"`.
With optional `data-tooltip-position` and `data-tooltip-class`

.control-bar

Backported from https://www.drupal.org/project/navigation/issues/3384996 📌 [PP1] Mobile Toolbar implementation Postponed
Just to be sure that everything works as expected on both mobile/desktop

During the development process, I had to break down the javascript into components. So at the moment some features do not work. Or they work bad :)
I also started work in a new branch since the previous one is outdated.

Please check and add child issues as needed!
Thanks

Thanks @finnsky for all the hard work, it's great and specially seeing SDC being used is really cool. It's a great reference and it has a lot of ideas, but sadly this MR can't be used as it is on the prototype for now. As we mentioned in the last meeting, we need to break this big issue in different ones to make it easier to review. But also to break this work in several smaller parts so the rest of the team is not blocked :)

@mherchel is going to take this on to unblock this, specially the most urgent things that need to be pushed like class names changes.

I'll keep working on breaking this issue in smaller pieces of work so everybody can take something to work on. @finnsky feel free to suggest smaller issues you've seen we could be working on.

For now, I've created the one for the font so we can centralize the discussion there: 🌱 [PLAN] Discuss using Inter font for the navigation Fixed . Updating the summary accordingly.

I just added 📌 Refactor classes Active to break down the huge efforts we need to do here.

Addressed some of the changes in 📌 Separate main components Fixed .

Referencing 📌 Prepend toolbar to toolbar variables Needs review .

Improve icon styles using the color from the CSS variable

I think this is already working like this unless I' missing something.

Combinet toolbar-button and separated tooltip in new MR

finnsky → closed merge request !68

finnsky → closed merge request !72

Hello all!

New MR with different approach:

I believe that in the future new elements with their own logic will appear in this toolbar. And keeping all the code in one file(sidebar.js) becomes pointless from the point of view of code maintainability and flexibility.

I believe that small independent components are easier to maintain and update.

I believe that the component called Popover in my merge request is technically not a menu because new components will appear in it in the future. An absolutely essential block with a cat. :) Or something different:
https://gyazo.com/c744e7cc0cf625cc73b7a834ddf287d4

This is also why the Button Toolbar and the Link Menu Toolbar are not one component. They are obviously different.

I believe that Drupal will be able to provide a tooltip component soon. Therefore, the tooltip should be independent of the sidebar.
https://www.drupal.org/project/drupal/issues/3197758 ✨ Create a new component: Toggletip RTBC

I believe that the use of variable fonts is very promising. Everything here is done in woff format (325kb now) So acceptable.

Implementation details:

Each component can dispatch Custom events via bubbling. And also listen to own events.
https://developer.mozilla.org/en-US/docs/Web/Events/Creating_and_trigger...
This leaves several scripts and logic at the sidebar.js

In fact, this is already implemented at the sidebar trigger level in:
js/admin-toolbar-wrapper.js

Some visual regressions(or progressions) exist now, but this is POC. All previous features works fine. And main thing:

We DON'T CLONE elements anymore. So semantic and a11y became much easier to maintain.

Please review!

Thanks! the code looks more readable, nice :)
In Drupal usually we try to optimize MR and patches for review since review is the main blocker in advancing issues. Anything you can do to make the review (and testing!) process easier will help get your changes in faster. That's the idea behind @ckrina and @mherchel suggestions.

I know it's a bit cumbersome but could you make your work in a dedicated JS refactoring issue I opened this one for this 📌 Extract tooltip JS component Active . As @ckrina mentioned first in #3 then again in #18, and @mherchel in #9 we need to have smaller issues to make it easier to review and commit. The font needs to be it's own issue and when you're refactoring the JS, try to avoid changing any css that is not necessary to change to make it work, we'll have a dedicated CSS clean-up issue.

Plan issues do not have code or patches in it, they are used as a 'epic' to have a single place to track a bunch of changes and are dedicated to strategy/status not actual implementation.

I do have a few comments on the code but I'd like to keep them to a dedicated JS refactoring issue, can you make your changes in 📌 Extract tooltip JS component Active and trying to keep only what's necessary for the JS changes please? (like limiting css changes, not including font, etc.)

Thanks a lot for the work so far, it looks great the only issue is that it's too big and it when reviewing it's hard to make sure all the features are working as intended and that there are no regressions.

From the points mentioned in the IS I only see that could be missing:

• Clean-up the CSS <-- This is too wide, so not sure what can be addressed here.
• Margin paddings do not support RTL <-- RTL has been addressed already
• The property tree is not flat and contains many cascades. <-- I think @finnsky has done a lot of improvements here, are there more planned?
• Icons should use the color from the CSS variable <-- I think @finnsky already addressed this in ✨ Split Navigation Button Component. Fixed .

@finnsky @mherchel do you have an idea of what do you think would be missing in here before closing this plan issue? We can open a new one in the future in we think there's a big effort needed.

Re #34

Yes also i plan to remove cloning from js and simplify menu.

Cool, then I'll move this to active again.

On my side i have nothing to add here. Probably it can be closed?

Great, thanks everybody for your help here! We can open a new plan for this in the future as we see fit.

Automatically closed - issue fixed for 2 weeks with no activity.