[Skeleton] Improve contrast on light themes #21122
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eps1lon
added
accessibility
|
@material-ui/core: parsed: +Infinity% , gzip: +Infinity% Details of bundle changes.Comparing: 5f2a237...a29d0e6 Details of page changes
|
|
@eps1lon Great topic! A couple of thoughts on the changes.
facebook.com low: high: material-ui.com low: high: pull request low: high: next.js https://static-tweet.now.sh/#incremental-static-generation low: I struggle to find more sources.
What do you think of this plan?
|
|
I literally can't see it on my screen. I have 100% vision (as of 2 years ago) and never experienced issues on this monitor. It obviously can't look bad if I can't see it
It is a breaking change since we changed from what value we read. If you relied on it, we break it. It still does not matter whether this is minor, major, big, small, etc. Breaking change in SemVer terms does not make a value statement. |
💯for working on the issue. I have updated my initial comments to take display conditions into account: the issue could also come from too much ambient light, not enough backlight, etc.
Yeah, ok, it's fair. So if we stop reading from the theme, it's no longer breaking? :) |
I'd say so. better be safe than sorry with the changelog. Especially since this is a lab component. A breaking change doesn't imply that it breaks your app. |
NMinhNguyen
reviewed
May 21, 2020
NMinhNguyen
reviewed
May 21, 2020
oliviertassinari
added
the
PR: needs revision
|
@eps1lon Would you mind if I explore the direction proposed in #21122 (comment)? My main point of concern would be: does the Facebook or Next.js examples have enough contrast to be visible on your screen? |
Yes I do mind. Every time I want to use or improve the component you have multiple points magically lined up. Could you open an RFC explaining the intention and direction you want to take the Skeleton? So far I have the strong feeling that all of this is post-hoc rationalization and knowing your intention upfront would help. I have no idea why a straight forward: "Make this perceivable" would trigger a whole redesign from your side because now it should like facebook or nextjs? This is again cherry-picking from your side. Just to summarize: You're actually telling me not seeing the component is my problem not the problem of the author. |
eps1lon
removed
the
PR: needs revision
|
This is unacceptable. Feel free to iterate further but in the current state this component is not usable:
|
|
@eps1lon Do you have the spec of your screen? Reference? How old? I would like to have a look at the online benchmark to see how it performs relative to an average screen of the market (it would help navigate if your screen is lower average or above and how much). Also, did you change the default display settings of the screen? Gaming mode, movie mode, etc? I had these options on an older display, it was creating distortions on the greyscale. It's a subtle tradeoff. Our direct users, developers, will likely have more high-end screen displays, especially to work with UI, than the average of the population, our end-users. We have direct feedback from the developers but indirect ones from the end-users. |
Lagicrus
referenced
this pull request
in BlobEmoji/blobs.gg
Jun 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Breaking Change: Skeleton no longer reuses
theme.palette.action.hoverfor its background color but fades to 16% of the primary text color.I cannot see the Skeleton in light mode on one of my monitors. Might be I'm getting old (even more reason to fix it), or my monitor is bad (also a reason a to fix it) but it's also using a color that is used for different purposes just because it had the same color:
:hoveris not necessarily a good semantic for placeholder UIs.In absence of a material design guide I'm fading the "on paper" color to 16%. Ideally the material guidelines would have values for these "loading"-states for primary/secondary and background color so that we could apply a systematic approach to
Skeleton,CircularProgressandLinearProgress.