removed opacity from roles.muted + replaced foregrounds in diff blob #153
Conversation
- removed opacity from roles.muted - replaced scale.gray.9 with fg.onEmphasis - in diffBlob all light text was replaced with fg.onEmphasis
🦋 Changeset detectedLatest commit: f8bcf1e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primitives/7UATd9hZFJ7YxL9pABon4z2eCwdF |
| diffBlob: { | ||
| addition: { | ||
| numText: get('fg.onEmphasis') | ||
| }, | ||
| deletion: { | ||
| numText: get('fg.onEmphasis') | ||
| }, | ||
| hunk: { | ||
| text: get('fg.onEmphasis') | ||
| }, | ||
| expander: { | ||
| icon: get('fg.onEmphasis'), | ||
| hoverIcon: get('fg.onEmphasis') | ||
| }, | ||
| commentButton: { | ||
| icon: get('fg.onEmphasis') | ||
| } |
There was a problem hiding this comment.
This is a bit tricky. Most of these diffBlob variables are deprecated and will get replaced by global variables. So I guess if we wanna have an exception only for HC, we would have to keep these variables for all the other themes?
There was a problem hiding this comment.
Do we already have a PR where they are replaced with Global variables? If not, can we replace them and then make the exception?
There was a problem hiding this comment.
Do we already have a PR where they are replaced with Global variables?
Not yet, but as soon as we start "cleaning up", we'll remove all the deprecated variables from Primer Primitives.
If not, can we replace them and then make the exception?
Hmm.. maaaaaybeee we could try using the "fallback mechanism" of CSS variables. Like
// only in dist/scss/colors_v2/_dark_high_contrast.scss
--color-diff-blob-expander-icon: #ffffff;
// somewhere on dotcom
color: var(--color-diff-blob-expander-icon, var(--color-fg-muted));Which means that the browser will use --color-diff-blob-expander-icon in HC since that variable is available, but fall back to --color-fg-muted for the rest of the themes? 🤔 I would've to test if that works. It also makes things more complex and harder to guess when a variable is used where. I guess it's a difficult ⚖️ balance to keep the system as simple as possible but also be able to fine tune it for special cases.
/cc @jonrohan @colebemis for thoughts 💭 .
There was a problem hiding this comment.
All of these changes are pointing to fg-onEmphasis, we could create one product variable to group all of these. something like diff.fg or something like that? Maybe it needs a little refactoring like giving up the colored numbers (they don't pass contrast on all modes anyway) and have everything a little more simple and systematic.
There was a problem hiding this comment.
Which means that the browser will use --color-diff-blob-expander-icon in HC since that variable is available, but fall back to --color-fg-muted for the rest of the themes?
The current Primitives setup enforces that all themes must have the same variables. Specifically, we cannot have a variable that exists in dark high contrast but not in the other themes.
We can change the Primitives setup but I actually think that rule has prevented a lot of bugs that could be hard to catch otherwise.
There was a problem hiding this comment.
We can change the Primitives setup but I actually think that rule has prevented a lot of bugs that could be hard to catch otherwise.
Yeah, I can imagine it will be a bit confusing having to track down which variables exist in which theme.
Ok, so in this case, if we want to have an exception only for HC, we would have to add again these app variables (like --color-diff-blob-expander-icon) to all themes. Or maybe better, try to what @auareyou suggested and reduce it to only a single app variable like diff.fg. I'll look into that in a separate PR.
There was a problem hiding this comment.
Update: Using diff.fg for all of these variables doesn't really work since only in HC they are the same, but in other themes they are different. Added these 3 variables back.
To increase the contrast in diffs I:
scale.gray.9withfg.onEmphasisfg.onEmphasis