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

Adjust grey tones (add new ones and change brightness values) #900

Merged
merged 10 commits into from
Jul 5, 2023

Conversation

LukasKalbertodt
Copy link
Member

This is in an attempt to unify grey tones across all our apps. I think this new 12 tone palette is a good, actually, given all the constraints we had and properties we wanted to optimize for.

Short recap: Tobira organically developed some grey tones during development; then Lisa's suggested a design for editor, studio and Tobira, which also had a palette of grey tones. Of course, those two sets of greys were not the same. So we decided to unify it and once and for all just set a fixed set of grey tones for these three applications, and for our applications in the future. This set shouldn't have too many colors, shouldn't have too few, should be close enough to Lisa's set so that it's easy looking up colors from the Figma, should be close to Tobira's set so that Tobira still works.

I decided for the values you see in the PR's diff. Here is a visualization of the brighter half of the light mode values:

image

Note how Tobiras grey0 and grey1 both map to neutral10. Both colors were never used next to one another (the contrast would be way too low). And I think the distinction there was really not necessary. So I think that's totally fine and even good that we have one tone fewer in Tobira!

Regarding naming: the PR as is uses the name of the closest Lisa grey. This has the advantage that if we check the designs in Figma, and we see an element with neutral200, we can just use our neutral20 color and get almost the same value, i.e. no need to think about anything. The disadvantage is that the numbers do not reflect brightness values at all; and we have no n15. I would much prefer the alternative naming shown in the image above, which would change the name of four colors. That way, we have n05, n10, n15, n20, n25, n30 and then we start with a step size of 10. That's much better than just randomly having n25 and n35, but no n15. Also, the brightness difference in the alternative names between n05, n10, n15, n20, n25, and n30 is almost constant (only increases slowly in one direction). And then the distance between n30, n40, n50 ... n90 is also fairly constant again. But the disadvantage: looking at Figma, seeing neutral200 or neutral300, those wouldn't map to neutral20 or neutral30 anymore, but to neutral15 and neutral25 respectively.

I also adjusted the dark mode values a bit such that they have more regular step sizes. The most notable change is that the series-block background got a bit darker. I don't mind that and might actually prefer it? But let me know what you think.


(Once everyone is happy with the colors, I will also refactor the frontend so that the fields of the COLORS constant are also called neutralXX.)

@LukasKalbertodt LukasKalbertodt marked this pull request as draft July 4, 2023 21:58
@github-actions github-actions bot temporarily deployed to test-deployment-pr900 July 4, 2023 22:03 Destroyed
@dagraf
Copy link
Collaborator

dagraf commented Jul 5, 2023

Thumbs up from me. Concerning the naming/numeration-issue, I have no strong opinion.

@LukasKalbertodt LukasKalbertodt marked this pull request as ready for review July 5, 2023 09:43
@LukasKalbertodt
Copy link
Member Author

Internally the grey tones were also accepted, so we will go ahead with them. Regarding the naming question: we decided for the alternative naming (closer to brightness values). The main argument was: we will work with the Figma design files for a while now, and then likely never again. So the "closer to Figma color names" is only a temporary advantage. And having more uniform names is nice.

@github-actions github-actions bot temporarily deployed to test-deployment-pr900 July 5, 2023 09:51 Destroyed
Copy link
Member

@owi92 owi92 left a comment

Choose a reason for hiding this comment

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

Thank you for taking this over. I mean, I was satisfied with my version, but I do see the benefits of (a) your version and (b) you being the one implementing it ;)

@owi92 owi92 merged commit 2e6b618 into elan-ev:master Jul 5, 2023
4 checks passed
@LukasKalbertodt LukasKalbertodt deleted the new-grey-tones branch July 5, 2023 10:44
@owi92 owi92 added changelog:user User facing changes changelog:dev Changes primarily for developers labels Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:dev Changes primarily for developers changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants