-
Notifications
You must be signed in to change notification settings - Fork 159
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
[full-ci] feat: introduce contrast colors in swatches #8563
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
926e6c6
to
d707d89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&-input input { | ||
border: 1px solid var(--oc-color-swatch-inverse-muted); | ||
} | ||
&-checkbox input { | ||
border: 2px solid var(--oc-color-swatch-inverse-muted); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icon of the current view mode is displayed in a white color now, which is a bit harder to see than the dark color before.
Stupid / naive me, thinking the tokens referencing values of other tokens would be respected at runtime. That's not the case. Only at build time. So the theme actually does need to set color vars for the contrast
swatch tokens. Grmpf, that's not what I wanted to achieve. But probably the way to go for now if we don't want to rebuild the tokens at runtime when switching / loading themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, the filter checkboxes are hidden in dark mode:
In light mode, the normal background color as well as the drop background color are white. TBH, I don't know why these colours differ in dark mode and why drops have this (ugly 😅 ) light background.
Can be solved by setting an input-border color value in dark mode which is not the same as the secondary background color. Did that in this PR. Checkbox needs some more fine tuning. Some of it is done in a PR from @hurradieweltgehtunter
@lookacat did you run |
d707d89
to
f5c575f
Compare
Results for e2e-tests oC10 https://drone.owncloud.com/owncloud/web/34019/11/1 💥 To see the trace, please open the link in the console ...
npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/34019/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-alice-2023-3-22-08-37-24.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/34019/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-brian-2023-3-22-08-37-53.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/34019/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-carol-2023-3-22-08-37-59.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/34019/tracing/public-link-alice-2023-3-22-08-40-09.zip |
e8e0c9b
to
5e833b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
<oc-button | ||
:id="`files-role-${roleOption.label.toLowerCase()}`" | ||
:id="`files-role-${roleOption.key}`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, that change causes the tests to fail because they look for the id
including the role name. So either we switch to data-test-id
for this, or revert it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thank you! I took the third option / a variation of the first option: I changed the id
prop again to use roleOption.name
because that is a lower-case html safe attribute already. With labels you never know for sure, since they're meant for the human eye. That's why I wanted to remove it from the template for the id in the first place.
The token fallback values are only calculated during build time. They are not relevant / evaluated when we set new colors to the css vars later on. Reason for that is simply that css vars can't reference other css vars but need their own value instead.
b37a138
to
63228b5
Compare
Results for e2e-tests oCIS https://drone.owncloud.com/owncloud/web/34022/12/1 💥 To see the trace, please open the link in the console ...
npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/34022/tracing/share-a-link-with-internal-role-alice-2023-3-22-09-39-32.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/34022/tracing/share-a-link-with-internal-role-alice-2023-3-22-09-47-04.zip |
63228b5
to
5aa0cb3
Compare
Kudos, SonarCloud Quality Gate passed! |
Description
This PR introduces a
contrast
token for all ourswatches
(i.e. our color variations in the design system). Their purpose is to define a high contrast color in relation to thedefault
token of the swatches. This way thecontrast
token can e.g. be used as text color if thedefault
token represents a background color.The
contrast
tokens are mainly used in three components of the design system:The main difference in those components now is that they don't use a generic text color anymore, but instead use the
contrast
color of the active variation. This brings us the flexibility of having light and dark swatches in parallel. Previously this was not possible because one single text color was used for all swatches/variations. Having a light brand color and a darkdanger
color was conflicting. One of the two always had bad readability for text.One additional goal was to completely get rid of the
inverse
swatch. Because, come on,inverse
to what exactly?! I'm not there, yet, there are still 4 usages of theinverse
swatch in the code. Might push that to a followup.Motivation and Context
More flexibility in theming.
How Has This Been Tested?
Well, it's about colors. Manually.
Mainly there are two areas of interest:
owncloud
default theme looks the same as before with this PR.brand
color. E.g. the json at the end of this PR description.Types of changes
Checklist:
Example theme.json with light brand color
In order to test this, create a new theme folder in
packages/web-runtime/themes
, insert the following content in atheme.json
file. Add logos as needed in an assets folder. Build web and set the theme path in thedocker-compose.yaml
file.