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

Summary clicks #539

Merged
merged 8 commits into from
Sep 14, 2021
Merged

Summary clicks #539

merged 8 commits into from
Sep 14, 2021

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Sep 1, 2021

Why are these changes introduced?

Fixes #383

What approach did you take?

Use user-select: none; Instead we're thinking of customizing the background color upon ::selection. Check the issue's comment for more info.

Other considerations

Demo links

Things to test

Try and select the text across the theme:

  • on buttons,
  • on buttons with secondary style,
  • text,
  • links,
  • variants,

Checklist

@ludoboludo
Copy link
Contributor Author

@Guillaumegranger1 would you maybe have time to review the approach ? Or let me know if I should get in touch with another designer 🙂
You can test using the links in the PR description.

@Guillaumegranger1
Copy link

I like this a lot. There only seems to be an issue when I chose the Background 1 Color scheme, because the highlight doesn't repurpose the color that's assigned to the text. Here's a screen recording of that.

@ludoboludo
Copy link
Contributor Author

I like this a lot. There only seems to be an issue when I chose the Background 1 Color scheme, because the highlight doesn't repurpose the color that's assigned to the text. Here's a screen recording of that.

Should be taken care of now. I was using the link color rather than text.

@ludoboludo
Copy link
Contributor Author

@svinkle if you want to have a look and see if there is any concerns in terms of color contrast.

@ludoboludo ludoboludo marked this pull request as ready for review September 8, 2021 15:25
@svinkle
Copy link
Member

svinkle commented Sep 8, 2021

Expect text color contrast to be at least 4.5:1:

  • #414141 / #d0d0d0: 6.61:1 (default text)
  • #ffffff / #5a5a5a: 6.89:1 (white on black)
  • 🛑 #ffffff / #5e75bf: 4.39:1 (blue product badge)

There may be other instances and I didn't test all default colors.

  1. Cmd + a to select everything
  2. Grab color values using macOS Digital Color Meter
  3. Test values with contrast-ratio.com

@ludoboludo
Copy link
Contributor Author

I took a look and it seems good to me with the default colors. When trying the sales badge, you get a couple different color values for the white text. If I take the whitest part then it seems good: rgb(88,113,194) and rgb(254,254,255) for a 4.57 contrast ratio 🤔
Not sure if it's fair to base ourselves on that though.

@tauthomas01 tauthomas01 self-assigned this Sep 10, 2021
Copy link
Contributor

@tauthomas01 tauthomas01 left a comment

Choose a reason for hiding this comment

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

This looks good 👍

@@ -140,6 +148,10 @@ h2,
background-color: transparent;
}

.button--secondary::selection {
background-color: rgba(var(--color-base-outline-button-labels), 0.3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, do we also need to apply this in the base.css?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.button--secondary in base.css is setting the variable --color-button-text to a new color.

And for .button::selection the background color is set to the variable --color-button-text. So it updates it automatically 👌

@kmeleta kmeleta self-requested a review September 10, 2021 19:22
Copy link
Contributor

@kmeleta kmeleta left a comment

Choose a reason for hiding this comment

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

LGTM

@ludoboludo
Copy link
Contributor Author

I took a look and it seems good to me with the default colors. When trying the sales badge, you get a couple different color values for the white text. If I take the whitest part then it seems good: rgb(88,113,194) and rgb(254,254,255) for a 4.57 contrast ratio 🤔
Not sure if it's fair to base ourselves on that though.

@svinkle let me know what you think. If you want to take another look maybe and give a 👍 or 👎

@ludoboludo ludoboludo merged commit f22de6f into main Sep 14, 2021
@ludoboludo ludoboludo deleted the summary-clicks branch September 14, 2021 15:38
olafghanizadeh pushed a commit to hyttefeber/dawn that referenced this pull request Mar 13, 2022
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disclosure - Double clicking the summary element selects it
5 participants