Skip to content

Aria component Progressbar contrast#4216

Merged
devongovett merged 5 commits intomainfrom
progress-bar-aria-component-contrast
Mar 17, 2023
Merged

Aria component Progressbar contrast#4216
devongovett merged 5 commits intomainfrom
progress-bar-aria-component-contrast

Conversation

@snowystinger
Copy link
Member

@snowystinger snowystinger commented Mar 16, 2023

Closes
Especially try in dark mode

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Mar 16, 2023

@rspbot
Copy link

rspbot commented Mar 17, 2023

<svg width={64} height={64} viewBox="0 0 32 32" fill="none" strokeWidth={strokeWidth}>
<circle cx={center} cy={center} r={r} stroke="var(--circle-color)" />
<circle cx={center} cy={center} r={r - (strokeWidth / 2 - .5)} stroke="var(--circle-color)" strokeWidth={1} />
<circle cx={center} cy={center} r={r + (strokeWidth / 2 - .5)} stroke="var(--circle-color)" strokeWidth={1} />
Copy link
Member

Choose a reason for hiding this comment

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

If we do a 1px line I think you can just have one circle?

Copy link
Member Author

Choose a reason for hiding this comment

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

a 1px line? you mean using the path element? or how do you get the empty area in between the two strokes? fill would do center of the circle and a path would still need two elements I think?

Copy link
Member

Choose a reason for hiding this comment

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

ah I think I was confused because it looks like the stroke is 2px even though it says 1px. it's definitely a lot thicker than the progress bar...

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this works but if you set strokeWidth to 0.5 and adjust the radius by 0.25 in either direction it looks right. Why would a .5px svg stroke look like a 1px box shadow?

Copy link
Member Author

Choose a reason for hiding this comment

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

o, you are right, it does look thicker than its prescribed 1px stroke... wonder if it is like that across all the browsers and all screen densities

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested across all three browsers i have, it looks like this gives it 1px. I've pushed the fix, thanks for catching that.

@rspbot
Copy link

rspbot commented Mar 17, 2023

ktabors
ktabors previously approved these changes Mar 17, 2023
Copy link
Collaborator

@ktabors ktabors 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 better.

@rspbot
Copy link

rspbot commented Mar 17, 2023

@rspbot
Copy link

rspbot commented Mar 17, 2023

@rspbot
Copy link

rspbot commented Mar 17, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM. Tested Chrome/FF/Safari dark and light modes

@devongovett devongovett merged commit 67f618d into main Mar 17, 2023
@devongovett devongovett deleted the progress-bar-aria-component-contrast branch March 17, 2023 22:16
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.

5 participants