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

feat: introduce style properties for disabled input #7553

Merged
merged 4 commits into from
Jul 30, 2024

Conversation

FrediWa
Copy link
Contributor

@FrediWa FrediWa commented Jul 16, 2024

Description

  • Adds variables for setting disabled background and text color. The text color applies to labels as well, as well as labels for radiobuttons and checkboxes.
  • Adds --vaadin-radio-button-disabled-background
  • Adds --vaadin-checkbox-disabled-background

Fixes #7482

Type of change

  • Feature

@FrediWa FrediWa changed the title feat: introduce color variables for disabled input feat: introduce style properties for disabled input Jul 16, 2024
@FrediWa FrediWa requested review from rolfsmeds and vursen July 16, 2024 08:40
Copy link
Contributor

@rolfsmeds rolfsmeds left a comment

Choose a reason for hiding this comment

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

Summary:

  • Let's only use the disabled-value-color for actual field values, not other disabled texts.
  • Let's introduce separate disabled background colors for CB and RB, similarly to the normal background colors.

}

:host([disabled]) [part='checkbox']::after {
color: var(--lumo-contrast-30pct);
color: var(--_disabled-value-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a breaking change: if someone is currently relying on the checkmark using --lumo--contrast-30pct, and overriding that property to a custom value, this change will break that.

So I'm a bit torn: on one hand it seems logical for the disabled checkmark to use the disable value color, but OTOH breaking change. Also, one might argue that the checkmark should not use disable value color since that is generally used for disabled text, whereas the checkmark is a selection indicator rather than a value per se (similar to the selection underline of a Tab for example).

@@ -27,6 +27,8 @@ registerStyles(
--_focus-ring-width: var(--vaadin-focus-ring-width, 2px);
--_selection-color: var(--vaadin-selection-color, var(--lumo-primary-color));
--_invalid-background: var(--vaadin-input-field-invalid-background, var(--lumo-error-color-10pct));
--_disabled-background: var(--vaadin-input-field-disabled-background, var(--lumo-contrast-10pct));
Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't realized checkbox and radiobutton uses a different disabled color compared to other fields, but here we are. This is not otherwise a problem, but if we want to add the disabled color properties to the opt-in global defaults (which we probably do want to do), it becomes a problem as CB+RB need a different value than other fields.

We might need to do a similar thing with CB and RB as we did for the normal background color, i.e. instead of using the general --vaadin-input-field-background, we introduced a separate --vaadin-checkbox-background, so that would mean a --vaadin-checkbox-disabled-background and --vaadin-radio-button-disabled-background.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I then just leave CB/RB out or should I introduce these specific variables and include them in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it wouldn't hurt to introduce them in the same PR.

}

:host([disabled]),
:host([disabled]) [part='toggle'] {
color: var(--lumo-disabled-text-color);
color: var(--_disabled-value-color);
cursor: default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Details summary is not really a value, so I don't think the disabled value color property makes sense here.

@@ -62,7 +63,7 @@ const item = css`

/* Disabled */
:host([disabled]) {
color: var(--lumo-disabled-text-color);
color: var(--_disabled-value-color);
cursor: default;
Copy link
Contributor

Choose a reason for hiding this comment

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

As with Details summary, this is not a field value, so let's not use the new prop here.

@@ -31,7 +32,7 @@ export const sideNavItemStyles = css`
}

:host([disabled]) [part='link'] {
color: var(--lumo-disabled-text-color);
color: var(--_disabled-value-color);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not an input value, so let's not use it here.

@@ -200,7 +201,7 @@ registerStyles(
:host([disabled]) {
pointer-events: none;
opacity: 1;
color: var(--lumo-disabled-text-color);
color: var(--_disabled-value-color);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not an input field value, so let's not use it here.

@@ -47,7 +48,7 @@ registerStyles(
}

:host([max-files-reached]) [part='drop-label'] {
color: var(--lumo-disabled-text-color);
color: var(--_disabled-value-color);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not an input field value, so let's not use it here.

color: var(--lumo-disabled-text-color);
-webkit-text-fill-color: var(--lumo-disabled-text-color);
color: var(red);
-webkit-text-fill-color: var(--_disabled-value-color);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Helper is not a value, so let's not use it here.

@FrediWa FrediWa requested a review from rolfsmeds July 22, 2024 05:38
@FrediWa FrediWa marked this pull request as draft July 24, 2024 10:54
@FrediWa FrediWa marked this pull request as ready for review July 25, 2024 07:54
@FrediWa FrediWa requested review from web-padawan and rolfsmeds and removed request for rolfsmeds and vursen July 25, 2024 08:17
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Let's also copy the new properties to this file so that we have defaults for them in globals export below these lines:

--vaadin-input-field-invalid-background: var(--lumo-error-color-10pct);
--vaadin-input-field-invalid-hover-highlight: var(--lumo-error-color-50pct);

--vaadin-input-field-disabled-background: var(--lumo-contrast-5pct);
--vaadin-input-field-disabled-value-color: var(--lumo-disabled-text-color);

Copy link

sonarcloud bot commented Jul 30, 2024

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.alpha7 and is also targeting the upcoming stable 24.5.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce style properties for disabled field colors
4 participants