Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActionList item outlines for high contrast theme #1856

Merged
merged 15 commits into from
Jan 14, 2022

Conversation

langermank
Copy link
Contributor

What are you trying to accomplish?

This adds a few more improvements to the ActionList, mainly:

What approach did you choose and why?

  • There is now an outline for :hover/:active that is transparent in most themes, except for the "high contrast" themes.

What should reviewers focus on?

  • We didn't use a border for the :hover/:active state to not have to change any sizes. Downside is that in Safari the rounded corners are missing.
  • The addition of a background color will overlay the vertical connections for tree-view. Is this okay, or do we need to address with a fix?
    image

Are additional changes needed?

  • ⚠️ Yes, when updating dotcom, primer/primitives needs to be bumped to 7.4.0 to include the new variables used in this PR.

@langermank langermank requested a review from a team as a code owner December 22, 2021 16:10
@langermank langermank requested a review from simurai December 22, 2021 16:10
@changeset-bot
Copy link

changeset-bot bot commented Dec 22, 2021

🦋 Changeset detected

Latest commit: a02806b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Co-authored-by: Vinicius Depizzol <vdepizzol@github.com>
Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@langermank The addition of a background color will overlay the vertical connections for tree-view. Is this okay, or do we need to address with a fix?

Hmm.. maybe ok since the background just appears on hover/active? If the "vertical connections" would be on top of the background, that might would feel awkward?

Another option could be to indent the background/border so that they don't cover the "vertical connections".

Screen Shot 2021-12-23 at 11 56 46

src/actionlist/action-list-item.scss Outdated Show resolved Hide resolved
src/actionlist/action-list-item.scss Outdated Show resolved Hide resolved
@vdepizzol
Copy link
Contributor

Another option could be to indent the background/border so that they don't cover the "vertical connections".

@langermank and I talked about this option in other scenarios such as navigationList. I think having ActionList-item elements filling the entire row with their target areas, and having hover/focus styles reflecting that is still our best choice going forward.

Regarding the vertical indentation lines/styles, I think we can improve those a little bit design-wise. When viewing multiple folders with single files inside, it becomes quite obvious they're too noisy. 🤔

50% {
box-shadow: inset 0 0 0 rgba(#000, 0.04);
// this could work if primitive supported rgb (not valid right now)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have to compromise on the fun interaction here for now. We could stack box-shadows to ensure we have a contrast border around the shadow that makes the item feel like its shrinking, but its not valid to use rgba() on a CSS variable that resolves as a hex. It would need to be rgb. 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@langermank sorry I missed this comment!

I think we could remove the inset box-shadow while keeping the scaling interaction in that case :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vdepizzol no worries! Okie, I think I got it 👍

@langermank langermank merged commit cbe3fb5 into main Jan 14, 2022
@langermank langermank deleted the actionlist-primitive-update branch January 14, 2022 00:30
@primer-css primer-css mentioned this pull request Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants