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

Improved accessibility for static labels and chips #8239

Conversation

Bonsai8863
Copy link
Contributor

Proposed changes

  • Changed text color for most labels/chips to pure black/white to improve readability
  • Added monochromatic setting in user profile to make labels/chips completely grayscale

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case (coverage and e2e)
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

I didn't write auto tests since they are mostly style changes

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.30%. Comparing base (c7a29d4) to head (80669ec).
Report is 320 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8239   +/-   ##
=======================================
  Coverage   66.29%   66.30%           
=======================================
  Files         597      597           
  Lines       60949    60949           
  Branches     6258     6262    +4     
=======================================
+ Hits        40407    40411    +4     
+ Misses      20542    20538    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch from f27aa82 to d8553e5 Compare September 4, 2024 12:17
@Bonsai8863 Bonsai8863 marked this pull request as draft September 4, 2024 17:49
@Kedae Kedae added the community use to identify PR from community label Sep 8, 2024
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch 4 times, most recently from 4d383d7 to 9fb1918 Compare September 11, 2024 15:02
@Bonsai8863 Bonsai8863 marked this pull request as ready for review September 11, 2024 15:22
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch 3 times, most recently from 77c49f4 to 4087e2d Compare September 17, 2024 12:53
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch 5 times, most recently from f137c15 to 50b38e5 Compare September 24, 2024 12:20
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch from 50b38e5 to 151dfdd Compare September 24, 2024 14:42
style={{
...classStyle,
color: theme.palette.chip.main,
borderColor: monochrome_labels ? theme.palette.background.accent : classStyle.borderColor,
Copy link
Member

Choose a reason for hiding this comment

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

Just having a thought right now : wouldn't it be better to have a attribute in the chip if we want the monochrome or the normal behavior and have the theme.palette.... inside all chips directly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we're using the MUI chip, so we could create a wrapper for it to determine monochromatic label status and apply the appropriate colors/styling.

Ideally, at least in my opinion, we'd extend the Theme interface and then be able to create a custom theme like light mode or dark mode then be able to switch to a monochromatic theme instead of all these ternary checks. At the moment, I think the platform is currently set to only handle dark or light mode themes.

Copy link
Member

Choose a reason for hiding this comment

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

That actually is a good idea, why don't we chose that path ?

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 talked to @Jipegien about it over email/conversation and he said to wait for feedback before implementing this expanded theme approach.

@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch 2 times, most recently from aea83f3 to ea12749 Compare October 1, 2024 11:41
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch from ea12749 to 7789251 Compare October 3, 2024 08:58
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch from 7789251 to e8e0d17 Compare October 16, 2024 13:49
@Bonsai8863 Bonsai8863 force-pushed the feature/monochrome_labels/static_labels branch from e8e0d17 to 80669ec Compare October 21, 2024 11:30
@Bonsai8863
Copy link
Contributor Author

Opening new PR for customizable themes concept instead of this one.

@Bonsai8863 Bonsai8863 closed this Dec 23, 2024
@Bonsai8863 Bonsai8863 mentioned this pull request Dec 23, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community use to identify PR from community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants