Problem/Motivation

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.

Proposed resolution

Prepend existing classes with the toolbar.
Define defaults.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork navigation-3392361

Command icon Show commands

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

ckrina created an issue. See original summary.

ckrina’s picture

Title: Prepend admin-toolbar to toolbar variables » Prepend toolbar to toolbar variables
Status: Active » Needs review

Here's the proposal to change the name of the variables from --color-focus to --toolbar-color-focus, prepending the module/component to the name.

A few things to discuss here and that I'm looking for feedback:

  1. I've used just "toolbar" here to simplify the resulting names, but maybe I should use admin-toolbar? Thoughts? To give some context, we might add a top contextual bar that can be called toolbar-whatever (no idea yet).
  2. I've changed where this is declared: from the :root element to the .admin-toolbar component 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-contextual or whatever we end up choosing, we declare the variables in 1 file for the module Navigation.
  3. I've defined the variables with the defaults following this great article from Lea Verou, but I haven't used the convention --_color she 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?

ckrina credited finnsky.

ckrina’s picture

Adding credit to @finnsky for his work in #3386927: [PLAN] Refactor CSS related to this.

finnsky’s picture

Status: Needs review » Needs work

Now 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.

ckrina’s picture

Assigned: ckrina » Unassigned

(waiting for more feedback before moving on)

ckrina’s picture

Status: Needs work » Needs review

I just opened a new MR because it was easier with all the changes needed. Things I've done that change from the previous MR:

  • I scoped the Tooltip variables for the Tooltip component itself. Since it's outside the .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.
  • I haven't defined the z-index because most elements that needed it were outside the .admin-toolbar itself too, so it didn't make sense to define them there. We could define them on :root but I'd prefer to do that in a follow-up and focus on approving&getting in the approach suggested here.
finnsky’s picture

Thanks 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.

kostyashupenko’s picture

Status: Needs review » Needs work
ckrina’s picture

Summary of the discussion on the meeting today:

  • We'll keep the :root as a place to define the variables because they will affect several components.
  • Replace --toolbar-variablename with --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.

kostyashupenko’s picture

Gonna fix everything

kostyashupenko’s picture

Status: Needs work » Needs review
ckrina’s picture

Status: Needs review » Needs work

Agreed with @finnsky: I would change --sidebar-header and --sidebar-width to --admin-toolbar-sidebar-header and --admin-toolbar-sidebar-width or similar too.

Thanks for the work, this is almost ready!

kostyashupenko’s picture

Status: Needs work » Needs review

  • ckrina committed aa61e0c7 on 1.x
    Issue #3392361: CSS variables refactor
    
ckrina’s picture

Status: Needs review » Fixed

Merging this so we can start working on the followups.

Feel free to open the follow-ups for:

  • Issue for adding z-index to the variables now they are on the :root.
  • Issue for discussing the default value defined, that can be easily overridden.
  • Move gradients into variables.

Status: Fixed » Closed (fixed)

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