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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from May 11, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/components/NcSelect/NcSelect.vue
View file
Open in desktop
Expand Up @@ -736,7 +736,7 @@
/**
* Array of options
*
* @type {Array<string | number | { [key: string | number]: any }>}

Check warning on line 739 in src/components/NcSelect/NcSelect.vue

View workflow job for this annotation

GitHub Actions / eslint 

Syntax error in type: Array<string | number | { [key: string | number]: any }>
*
* @see https://vue-select.org/api/props.html#options
*/
Expand Down Expand Up @@ -781,7 +781,7 @@
*
* The `v-model` directive may be used for two-way data binding
*
* @type {string | number | { [key: string | number]: any } | Array<any>}

Check warning on line 784 in src/components/NcSelect/NcSelect.vue

View workflow job for this annotation

GitHub Actions / eslint 

Syntax error in type: string | number | { [key: string | number]: any } | Array<any>
*
* @see https://vue-select.org/api/props.html#value
*/
Expand Down Expand Up @@ -945,6 +945,9 @@
--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 •
edited

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

--vs-disabled-color: var(--color-text-maxcontrast);
--vs-disabled-cursor: not-allowed;
Comment on lines +948 to +950
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.

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

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


/* Borders */
--vs-border-color: var(--color-border-maxcontrast);
Expand Down Expand Up @@ -998,13 +1001,13 @@
margin-right: 2px;
}

&.vs--open .vs__dropdown-toggle {
border-color: var(--color-primary);
&.vs--open .vs__dropdown-toggle:not([disabled]) {
border-color: var(--color-primary-element);
Antreesy marked this conversation as resolved.
Show resolved Hide resolved
border-bottom-color: transparent;
}

&:not(.vs--open) .vs__dropdown-toggle:hover {
border-color: var(--color-primary);
&:not(.vs--open) .vs__dropdown-toggle:hover:not([disabled]) {
border-color: var(--color-primary-element);
}

&--no-wrap {
Expand All @@ -1019,7 +1022,7 @@
.vs__dropdown-toggle {
border-radius: 0 0 var(--vs-border-radius) var(--vs-border-radius);
border-top-color: transparent;
border-bottom-color: var(--color-primary);
border-bottom-color: var(--color-primary-element);
}
}
}
Expand Down