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

@clayui/css: Buttons increase specificity of .btn:disabled #5469

Open
pat270 opened this issue Apr 13, 2023 · 6 comments
Open

@clayui/css: Buttons increase specificity of .btn:disabled #5469

pat270 opened this issue Apr 13, 2023 · 6 comments
Labels
comp: clay-css Issues related to Clay CSS type: bug Issues reporting that Component is not doing what should be done

Comments

@pat270
Copy link
Member

pat270 commented Apr 13, 2023

If there is a single .btn:disabled state, we need to apply the styles to every variant due to .btn:disabled being declared before the variants like .btn-primary. It makes it difficult to apply the disabled state since we need to apply it on every variant or else states like .btn-primary:hover will win when hovering over a disabled button.

We can increase specificity directly in the .btn:disabled selector or we can declare the selector later in the file after the variants.

@pat270 pat270 added type: bug Issues reporting that Component is not doing what should be done comp: clay-css Issues related to Clay CSS labels Apr 13, 2023
@ethib137
Copy link
Member

There is another option, but I'm not sure you'll like it... 😉

.btn {
    background-color: var(--btn-default-background-color);

    &:hover {
        --btn-default-background-color: var(--btn-hover-background-color);
    }

    &:active {
        --btn-default-background-color: var(--btn-active-background-color);
    }

    &:disabled {
        --btn-default-background-color: var(--btn-disabled-background-color);
    }
}

.btn-primary {
    --btn-default-background-color: blue;
    --btn-hover-background-color: darkBlue;
    --btn-active-background-color: var(--btn-hover-background-color);
}

This way specificity and order is set once and new variants only override what they are concerned with.

@pat270
Copy link
Member Author

pat270 commented Apr 14, 2023

@ethib137 I'm not totally against this other than the breaking changes part. It still runs into the same problem where we need to declare the disabled colors on .btn-primary. In this issue, I'd like to declare it on .btn:disabled without having to redeclare colors on variants.

@ethib137
Copy link
Member

Hey @pat270 Yeah, I think this would solve the problem of having to redeclare the disabled colors for the variants. Check out this code pen example: https://codepen.io/ethib137/pen/zYmqzqV.

Additionally I don't think this would have to be a breaking change. We could still use the existing sass maps to generate the type of markup I'm writing in the codepen. Since the sass maps are our api it woudn't need to be a breaking change, right?

@pat270
Copy link
Member Author

pat270 commented Apr 17, 2023

@ethib137 Thanks for the example, that would indeed work. The underlying CSS is being changed, but I don't think it would affect people overwriting our components via a separate CSS file. It just adds some extra custom properties for users using it the traditional way. I think I need to try and see if it would break some existing themes before committing.

If we do decide to do this, how do we communicate we are making this change? If we decide here then we would have to commit to the whole project.

@ethib137
Copy link
Member

Yeah, it would be important to communicate, since one of the benefits of this is it would make it easier to override our css since it would reduce the specificity needed of our selectors. This is also a danger since it could result in some styles being overridden by custom themes that the dev did not intend. Is this a change we would have to wait for a major version to move to?

If we decide here then we would have to commit to the whole project.
Yeah, I think that could be a huge benefit, but we have to think carefully of what this could possibly break. Testing with some other themes should help with this.

@liferay-platform-experience
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: clay-css Issues related to Clay CSS type: bug Issues reporting that Component is not doing what should be done
Projects
None yet
Development

No branches or pull requests

3 participants