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

fix(NcSelect): Fix disabled state of NcSelect with dark mode #4079

Merged
merged 1 commit into from May 11, 2023

Conversation

nickvergessen
Copy link
Contributor

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
grafik Bildschirmfoto vom 2023-05-10 09-57-25
Bildschirmfoto vom 2023-05-10 10-08-40 Bildschirmfoto vom 2023-05-10 10-09-31

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen added bug Something isn't working 3. to review Waiting for reviews feature: select Related to the NcSelect* components accessibility Making sure we design for the widest range of people possible, including those who have disabilities labels May 10, 2023
@nickvergessen nickvergessen added this to the 8.0.0 milestone May 10, 2023
@nickvergessen nickvergessen self-assigned this May 10, 2023
@nickvergessen
Copy link
Contributor Author

Found while fixing nextcloud/server#38170

Copy link
Contributor

@Antreesy Antreesy left a 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);
Copy link
Contributor

@Antreesy Antreesy May 10, 2023

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

src/components/NcSelect/NcSelect.vue Show resolved Hide resolved
@nickvergessen
Copy link
Contributor Author

/backport to stable7

Comment on lines +948 to +950
--vs-disabled-bg: var(--color-background-dark);
--vs-disabled-color: var(--color-text-maxcontrast);
--vs-disabled-cursor: not-allowed;
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

Copy link
Contributor

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

@nickvergessen nickvergessen merged commit 4aab112 into master May 11, 2023
16 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/fix-dark-mode-NcSelect branch May 11, 2023 12:20
This was referenced May 12, 2023
@Pytal Pytal mentioned this pull request Jun 14, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews accessibility Making sure we design for the widest range of people possible, including those who have disabilities bug Something isn't working feature: select Related to the NcSelect* components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants