Closed (fixed)
Project:
Navigation
Version:
1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
7 Oct 2023 at 14:36 UTC
Updated:
9 Nov 2023 at 11:24 UTC
Jump to comment: Most recent
Right now the variables are mostly the ones taken from Claro, but they follow a naming that can easily collapse with any other design system name. Let's define the default classes for the Toolbar in a way that they can be easily overridden and that provide a default if they aren't.
Prepend existing classes with the toolbar.
Define defaults.
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
ckrinaHere's the proposal to change the name of the variables from
--color-focusto--toolbar-color-focus, prepending the module/component to the name.A few things to discuss here and that I'm looking for feedback:
admin-toolbar? Thoughts? To give some context, we might add a top contextual bar that can be called toolbar-whatever (no idea yet).:rootelement to the.admin-toolbarcomponent itself. The goal is to scope the variable declaration to the component itself so we don't expose it to everything. I've left it as an independent file because if we add another component called.toolbar-contextualor whatever we end up choosing, we declare the variables in 1 file for the module Navigation.--_colorshe mentions that prepends with an underscore to evoke a private variable because I don't think it's a good call to adopt this for core at this point.Thoughts?
Comment #5
ckrinaAdding credit to @finnsky for his work in #3386927: [PLAN] Refactor CSS related to this.
Comment #6
finnsky commentedNow i see that z-indexes on components are pretty generic.
We need to transform z-indexes to variables and control them from one single place.
`--z-index-toolbar`: 500;`
`--z-index-tolltip`: 601;`
etc.
Also rebase needed i guess.
Comment #7
ckrina(waiting for more feedback before moving on)
Comment #10
ckrinaI just opened a new MR because it was easier with all the changes needed. Things I've done that change from the previous MR:
.admin-toolbar, the variables were outside of scope, and since it's supposed to be its own element it's better to have the variables defined there. I've kept the same naming used everywhere else though. I think we can refactor the variable names when we can really make this component independent..admin-toolbaritself too, so it didn't make sense to define them there. We could define them on:rootbut I'd prefer to do that in a follow-up and focus on approving&getting in the approach suggested here.Comment #11
finnsky commentedThanks for your work here!
Some thoughts.
I see a small logical error here. Or double reinsurance :)
If we add complex prefixes to variables, then there is nothing wrong if they are assigned in the root element.
If we assign variables in the .admin-toolbar context, then why add complex prefixes to them? Variables like --color-blue in Olivero or Claro assigned in the root will still be overwritten in our sidebar. And there will be no collisions
So we can simplify it a little bit. Either adding 100% unique variable names to the root or adding simple variable names to the .admin-toolbar context
Also i added some notes in MR.
Comment #12
kostyashupenkoComment #13
ckrinaSummary of the discussion on the meeting today:
:rootas a place to define the variables because they will affect several components.--toolbar-variablenamewith--admin-toolbar-variablename, so we prepend the variables with "admin-toolbar" to make it more obvious that it's for the admin elements.Edit: And I've forgot to mention that we'll move the discussion about the conflict with simple variable names to a follow-up so we can get this in and avoid all the code collisions, and just discuss about the defaults defined on the variables later on that follow-up.
Comment #14
kostyashupenkoGonna fix everything
Comment #15
kostyashupenkoComment #16
ckrinaAgreed with @finnsky: I would change
--sidebar-headerand--sidebar-widthto--admin-toolbar-sidebar-headerand--admin-toolbar-sidebar-widthor similar too.Thanks for the work, this is almost ready!
Comment #17
kostyashupenkoComment #19
ckrinaMerging this so we can start working on the followups.
Feel free to open the follow-ups for: