feat(components): Re-separate Chip from Chips w/Background Color Fixes

ZakaryH commented 5 days ago

Motivations

Bringing back the changes we tried to make to separate Chip from Chips in another PR that we reverted due to some background color issues with an instance of Chips using Chips/Chip as the children.

Changes

Added

the ability to override the appearance of a Chips "active" Chip with

--public-chip-base-active-bg-color for the resting background color
--public-chip-base-active-hover-bg-color for the hover/focus color (issue with focus styles right now though)
--public-chip-base-active-content-color for the text and separator line that can appear on a Chip

question: do we need one more for the active content in a hover state?

also, is the name too wordy? it was originally chip-base-bg-color etc. but I thought that was somewhat unintuitive in the context of Chip where it will not style the unselected chips, only the selected/active chips will be overridden. this is because under the hood they are the "base" variant.

it's a bit awkward because a standalone Chip it doesn't have anything to do with being active at all. it's because Chips is now using standalone Chip within InternalChip, and there it is being set as a base.

I could be convinced to simply go with chip-base-bg-color since it does affect the standard base Chip . I simply thought it was confusing, or expecting consumers to know too much about the internal implementation within the Chips context.

Changed

it might look like a lot, however most of it is from the other PR. the only new additions in this PR are:

fixing the cursor for disabled divs that are rendered when there's no onClick
removing unused classnames from InternalChip.css
adding 3 --public CSS vars for people to change the active (technically base) appearance
making the Chips unselected Chip be subtle and the active ones base
changing how we're using CSS vars in Chip.css to hopefully make it a bit cleaner

Deprecated

Removed

Fixed

Security

Testing

same testing as https://github.com/GetJobber/atlantis/pull/1995

on top of that, try to set up some Chips with overrides eg.

import { Chips, Chip } from "@jobber/components/Chips";

...
// try all types
<Chips 
  type="dismissible"
>
  <Chip label="whatever" value="whatever" />
  <Chip label="invalid" value="invalid" invalid />
  <Chip label="disabled" value="disabled" disabled />
  <Chip label="selected" value="select me" />
</Chips>

and wrap that instance in something that does

      <div
        style={
          {
            "--public-chip-base-active-bg-color":
              "var(--color-interactive--subtle)",
            "--public-chip-base-active-hover-bg-color":
              "var(--color-interactive--subtle--hover)",
            "--public-chip-base-active-content-color": "var(--color-pink)",
          } as React.CSSProperties
        }
      >
        ...
    </div>

or you can do it with a proper stylesheet too. probably good to try that too. assuming you scope it adequately, you should be able to provide 2 different sets of overrides to 2 different Chips implementations on the same page

then, when you select a Chip - it should apply the overrides on that active chip. the other should remain untouched, and disabled/invalid also doesn't change. unselected chips should have the subtle appearance/variant.

the truncation should all still work as intended with the new setup mirroring the background color of the chips as they change.

disabled Chips that don't have an onClick get rendered as divs and so we can't rely on cursor: auto on them anymore. both button and div Chip when disabled should have just the little arrow cursor.

Changes can be tested via Pre-release

In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

cloudflare-workers-and-pages[bot] commented 5 days ago

Deploying atlantis with Cloudflare Pages

Latest commit:	`23030f5`
Status:	✅ Deploy successful!
Preview URL:	https://68535c92.atlantis.pages.dev
Branch Preview URL:	https://job-105066-chip-bg-colors.atlantis.pages.dev

View logs

ZakaryH commented 5 days ago

last commit is a bit of cleanup we should have done in the Separate Chip from Chips PR when we started using standalone Chip

https://github.com/GetJobber/atlantis/pull/1995/files#diff-94e5c37e1604458d3cff18d0ffd0612251c263234bb46b8b72d0a307c95e86fbL21-L37

a bunch of css rules and classnames were left in the InternalChip.css file and nothing was referencing them so I removed all of the unused rules and selectors that cannot happen anymore

there are still a few other places referencing icon button wrapper and input so I've kept those

ZakaryH commented 5 days ago

last commit changes how the background, border and hover variations of those are set

instead of setting it multiple times in each variant eg.

.subtle { background-color: var(--some-color); }

we're going to have a single border, and background-color in the .chip class that refers to a --public var for their colors, each having their own. we were never changing the border width or type, only the color so that's the only variable that each variant will manipulate.

then, in a single hover/focus-visible selector, we have another border and background-color - which will have priority over the base with the higher specificity

lastly, the concept of the gradient variables became redundant. there was no point in tracking that separately. the whole purpose of the gradient is to match the background color, so now it simply mirrors whatever the background is set to with the public variables used to change the hover/non-hover background color

I think this is much simpler, and hopefully easier to follow. hover and focus-visible selectors will only modify the hover public variable. non-hover selectors will modify the border color and non-hover background public variable.

this does raise the question of if there should be a border-hover equivalent to allow changing that in the same way you can now change the background color on hover.

one drawback to this approach is that since everything looks at the public color, you would not be able to surgically specify one color for a subtle Chip, and then a different override for an invalid Chip. if that's something we care about, then we'd need to add public vars for each and change things up a bit.

hmm. only base is affected when a root level override is provided.

"Styles for a directly targeted element will always take precedence over inherited styles, regardless of the specificity of the inherited rule."

so in order for the overrides to work, you can't have something in the directly targeted class to compete with. interesting.

ZakaryH commented 15 hours ago

noticed that for single & multiselect Chips on this branch if you focus on the chip it has a different appearance from if you hover on it. this isn't exactly what we want. I was hoping the fix would be to merge the focus-visible and hover styles, however it's a bit more complicated than that.

it's a result of the separation PR, where we are giving the InternalChip for those two variants a tabIndex={-1}. this is causing it to not be focusable. you can see the dismissible AKA selection variant doesn't exhibit this behavior because it is focusable.

it seems to be a descendant that is getting focused, and we had to set the tabIndex for some other issue I recall reading a note about. this makes it hard to set the color of the parent.

GetJobber / atlantis