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

adds an index above each scale step #40

Merged
merged 4 commits into from Jun 29, 2022

Conversation

bryanberger
Copy link
Contributor

@bryanberger bryanberger commented Jun 23, 2022

Adds a string above each scale step to indicate which index in the scale.colors array the color is. I opted to wrap the existing scale ZStack in order to separate it from that container (so the points don't interfere with it.

Screen Shot 2022-06-22 at 8 46 59 PM
)

ref: #32

@bryanberger
Copy link
Contributor Author

bryanberger commented Jun 23, 2022

I also tried adding these below the scale steps but it seemed too cluttered down there. Thoughts?
Screen Shot 2022-06-22 at 8 52 02 PM

Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Love this! 💖

I left a couple of minor comments about using the Box component and making the active index bold. Happy to merge after those are addressed

src/pages/scale.tsx Outdated Show resolved Hide resolved
src/pages/scale.tsx Outdated Show resolved Hide resolved
src/pages/scale.tsx Outdated Show resolved Hide resolved
@bryanberger
Copy link
Contributor Author

bryanberger commented Jun 24, 2022

Thanks for the <Box/> tips @colebemis!

I ran into a slight issue, because the color steps have a variable width, we run into an alignment issue when lifting a div above them.

Screen Shot 2022-06-23 at 10 48 11 PM
Screen Shot 2022-06-24 at 10 20 36 AM

I might try and add these to the :before content, but I prefer not to introduce more negative absolute positioning.

Also...we solved the variable width in our fork by rotating the contrast spans vertically instead (so the width doesn't fluctuate based on contrast score text, and also renamed AA Large to just AA+ for brevity) I can add that to this PR as well if you think it's a good direction.
Screen Shot 2022-06-24 at 10 16 59 AM

@colebemis
Copy link
Contributor

image

I love this solution! Feel free to add it to this PR 💖

@bryanberger
Copy link
Contributor Author

Latest screenshot with mentioned changes:

Screen Shot 2022-06-27 at 10 18 55 PM

<div
style={{
display: 'flex'
<div style={{height: 8}}></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open for feedback here. We need a bit more vertical breathing room in the grid layout, opted not to adjust the gap between everything, but instead just add some space at the top.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems okay to me 👍

backgroundColor: 'var(--color-text)',
borderRadius: 2
},
'&::after': {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't really love the semantics here of using a :before and :after that are both top aligned, but I tried a few different approaches and this seems the most stable and responsive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, I'm fine with it

@@ -39,7 +39,7 @@ export function getRange(type: Curve['type']) {
}

export function getContrastScore(contrast: number) {
return contrast < 3 ? 'Fail' : contrast < 4.5 ? 'AA Large' : contrast < 7 ? 'AA' : 'AAA'
return contrast < 3 ? 'Fail' : contrast < 4.5 ? 'AA+' : contrast < 7 ? 'AA' : 'AAA'
Copy link
Contributor

Choose a reason for hiding this comment

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

@bryanberger is there a authority which regulates/determines this score? can you please share?

I tried searching on my own, only found this https://www.w3.org/WAI/WCAG21/Understanding/contrast-minimum.html

I'd love to know more about accessibility on web.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a good reference: https://webaim.org/articles/contrast/

Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

This is amazing. Thank you, @bryanberger! 💖

@colebemis colebemis merged commit 6f55d45 into primer:main Jun 29, 2022
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.

None yet

3 participants