-
Notifications
You must be signed in to change notification settings - Fork 90
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
fix(NcSelect): Fix disabled state of NcSelect with dark mode #4079
Conversation
nickvergessen
commented
May 10, 2023
Before | After |
---|---|
Background is not our background color and therefor also used in dark mode | background is our color variable so perfectly fine in dark mode |
Signed-off-by: Joas Schilling <[email protected]>
Found while fixing nextcloud/server#38170 |
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.
🦭
@@ -945,6 +945,9 @@ body { | |||
--vs-state-disabled-color: var(--color-text-maxcontrast); | |||
--vs-state-disabled-controls-color: var(--color-text-maxcontrast); | |||
--vs-state-disabled-cursor: not-allowed; | |||
--vs-disabled-bg: var(--color-background-dark); |
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.
Aliases --vs-state-disabled-*
should update this value, but maybe it's not hapenning because of the css rules order, so I guess, it's legal to override here
/backport to stable7 |
--vs-disabled-bg: var(--color-background-dark); | ||
--vs-disabled-color: var(--color-text-maxcontrast); | ||
--vs-disabled-cursor: not-allowed; |
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 --vs-state-disabled-*
variables are already set above and are aliased to these variables internally https://github.com/nextcloud-deps/vue-select/blob/9294fdcbf39380fd5cc766144b18b6f86cf107f4/src/css/global/states.css#L13C35-L15
Is this solvable without adding non-publicly documented variables https://vue-select.org/guide/css.html#available-css-variables-3-18?
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.
I have no clue why, but it fixes it. As per #4079 (comment) it could be the order, since --vs-disabled-bg: var(--vs-state-disabled-bg);
is not a permanent redirect, but it writes the currently value of the second var into the first one. So maybe we only replace the --vs-state-disabled-bg
after that and therefore also need to overwrite the first var
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.
Or well:
https://github.com/nextcloud/nextcloud-vue/blob/93f9ab752f7d64dd14afbe6d549f0e1fab01d6ae/src/components/NcSelect/NcSelect.vue#L525
I'm pretty sure that is exactly what is going on
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.
I think this happens because:
--vs-disabled-bg: var(--vs-state-disabled-bg);
from vue-select if on the :root
selector.
And our --vs-state-disabled-bg: var(--color-background-dark);
is on the body
selector.
So when --vs-disabled-bg
is set, the --vs-state-disabled-bg
variable holds the old value.
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.
So I think it has the same cause as this one: nextcloud/server#36462