Skip to content

Use pseudo-class too for disabled check/radio#29740

Merged
XhmikosR merged 4 commits intotwbs:masterfrom
jeromelebleu:fix-29739
Dec 6, 2019
Merged

Use pseudo-class too for disabled check/radio#29740
XhmikosR merged 4 commits intotwbs:masterfrom
jeromelebleu:fix-29739

Conversation

@jeromelebleu
Copy link
Copy Markdown
Contributor

@jeromelebleu jeromelebleu commented Nov 26, 2019

In order to consider check/radio inputs inside a disabled fieldset, this first use the :disabled pseudo-class. By the way, the URL of the relevant issue on M$ Edge seems broken or needs at least an account to track its progress...

fixes #29739

@jeromelebleu jeromelebleu requested a review from a team as a code owner November 26, 2019 15:31
@MartijnCuppens
Copy link
Copy Markdown
Member

MartijnCuppens commented Nov 26, 2019

Maybe we could link to #28247 instead?

MartijnCuppens
MartijnCuppens requested changes Nov 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

This should also be changed in _form-file.scss

@jeromelebleu
Copy link
Copy Markdown
Contributor Author

jeromelebleu commented Nov 27, 2019

Thanks for the link and the review. I have updated the patch to use the suggested workaround for Edge instead to keep only the :disabled pseudo-class for the ruleset. However, I don't have Edge to validate that it is still working on it. Could someone check that please?

@jeromelebleu jeromelebleu force-pushed the fix-29739 branch 2 times, most recently from c4258c4 to d75d5ed Compare November 27, 2019 22:15
@ysds
Copy link
Copy Markdown
Contributor

ysds commented Nov 28, 2019

I am not a fan of such CSS hack 😕. I think the previous change (both :disabled and [disabled] ) is better.

@jeromelebleu
Copy link
Copy Markdown
Contributor Author

jeromelebleu commented Nov 28, 2019

@ysds I agree with you, besides it seems to come out of nowhere... I have switched to this hack as it has been tested on Edge and implemented in other projects regarding #28247, and because I cannot confirm that using both :disabled and [disabled] will work on it too. But it is as you want for sure.

@MartijnCuppens
Copy link
Copy Markdown
Member

MartijnCuppens commented Nov 28, 2019

I agree with @ysds here

@ysds
Copy link
Copy Markdown
Contributor

ysds commented Nov 28, 2019

I want to include this to 4.4.1 if we can work quickly.

@jeromelebleu
Copy link
Copy Markdown
Contributor Author

jeromelebleu commented Nov 28, 2019

I have made changes accordingly, if someone can confirm that it is working on Edge please.

@ysds
Copy link
Copy Markdown
Contributor

ysds commented Nov 28, 2019

I created a PR for v4: #29762

@ysds
Copy link
Copy Markdown
Contributor

ysds commented Nov 28, 2019

So no longer need to rush this PR :)

@ysds ysds mentioned this pull request Nov 28, 2019
Merged
@ysds
Copy link
Copy Markdown
Contributor

ysds commented Nov 29, 2019

I checked the following test cases:

@ysds ysds mentioned this pull request Dec 3, 2019
Closed
@XhmikosR XhmikosR merged commit 2a6ba75 into twbs:master Dec 6, 2019
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
@jeromelebleu @olsza
Use pseudo-class too for disabled check/radio (twbs#29740)
fe5f5a9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MartijnCuppens MartijnCuppens MartijnCuppens approved these changes

+1 more reviewer

@ysds ysds ysds approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

css no-backport-to-v4 v5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Check/radio doesn't consider disabled fieldset

4 participants

@jeromelebleu @MartijnCuppens @ysds @XhmikosR