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

Remove deprecated govuk-button--disabled class #3557

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

querkmachine
Copy link
Member

Removes the .govuk-button--disabled class that was deprecated in 4.6.0. Closes #2681.

Changes

  • Removes the .govuk-button--disabled selector and the deprecation comment from the stylesheet.
  • Removes the disabled link example from the review app.
  • Removes the test associated with the disabled link example.
  • Updates the Nunjucks macro to no longer add the .govuk-button--disabled class to disabled buttons.
  • Updates the Nunjucks macro documentation to indicate that the disabled parameter has no affect on a elements.
  • Updates tests to no longer check for the existence of the .govuk-button--disabled class.

Thoughts

We also have a .govuk-button[disabled="disabled"] selector defined in there. I'm not sure why this exists alongside .govuk-button[disabled], as they have the same specificity and disabled is a boolean attribute (the presence of a value is meaningless).

.govuk-button[disabled] alone would seem to suffice. Should we also remove .govuk-button[disabled="disabled"]? I don't think doing so would require a deprecation or breaking change.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3557 April 26, 2023 16:26 Inactive
@querkmachine querkmachine self-assigned this Apr 26, 2023
@querkmachine querkmachine requested a review from a team April 26, 2023 16:27
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3557 April 26, 2023 16:36 Inactive
@querkmachine
Copy link
Member Author

The changelog might be a bit awkward as there's already a section regarding changes to disabled buttons. I've tried to merge these changes into it rather than having a separate, similarly titled section.

@querkmachine querkmachine marked this pull request as ready for review April 26, 2023 16:40
@owenatgov owenatgov self-requested a review April 26, 2023 16:49
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

Looks good to me 💪🏻

I'm unsure if we should also remove disabled="disabled". Nothing struck me as a reason it was there in the commit history that I was able to uncover. My guess is this is a failsafe for incorrect implementations? I'm leaning towards not deleting it but it also feels like that's just kicking the can down the road. Maybe we should just get rid of it 👀

@querkmachine
Copy link
Member Author

My guess is this is a failsafe for incorrect implementations?

Unless I'm having a late afternoon brainfart (not unheard of!) I'm not aware of any implementation that .govuk-button[disabled="disabled"] would match and that .govuk-button[disabled] wouldn't also match.

Even if teams have added their own styles for either selector, the value-d option has no greater specificity than the value-less one, and a team's own styles will trump both of them on the cascade alone anyway.

I feel pretty safe taking it out, I'm pretty confident it can't break anything.

@36degrees
Copy link
Contributor

I agree with your conclusion – anything that's covered by button[disabled="disabled"] would already be covered by button[disabled]. So I think we can safely get rid of it.

I'm not 100% sure about merging the two entries under a single changelog entry – I'm mainly wondering if the second one might get missed.

I do think we should refer back to it's deprecation, like we did for the previous major release. If users have been acting on our deprecations they shouldn't have to do anything – adding this would help them make that connection.

This selector is seemingly redundant as all elements that would match it would also match `.govuk-button[disabled]` at the same specificity.
@querkmachine querkmachine force-pushed the remove-button-disabled-class branch from f67c3c0 to 704a617 Compare April 27, 2023 08:54
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3557 April 27, 2023 08:55 Inactive
@querkmachine querkmachine merged commit a53c6a4 into main Apr 27, 2023
@querkmachine querkmachine deleted the remove-button-disabled-class branch April 27, 2023 10:02
@romaricpascal romaricpascal changed the title Remove deprecated .govuk-button--disabled class Remove deprecated govuk-button--disabled class Nov 27, 2023
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove deprecated govuk-button--disabled class
4 participants