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

Correctly style disabled warning button #1051

Merged
merged 4 commits into from
Nov 20, 2024
Merged

Conversation

paulrobertlloyd
Copy link
Contributor

Description

  • Simplifies how disabled buttons are styled
  • Adds examples of all button variants in disabled state on example page
  • Updates backstop images

Fixes #1034.

I’m not really sure why so much CSS is being used to style disabled buttons! We appear to give disabled buttons background colours that have already been defined, and so much else, when all we need to do is give buttons with nhsuk-button--disabled class or disabled 0.5 opacity, change to the default cursor and remove pointer-events.

Maybe I’m missing something? (I note that GDS do the same for govuk-frontend so might be something to ask them about this on X-GOV Slack)?

Checklist

@paulrobertlloyd paulrobertlloyd changed the title Warning button disabled Correctly style disabled warning button Oct 22, 2024
@paulrobertlloyd paulrobertlloyd added buttons Code improvements Any changes that improve but do not alter the appearance of the site labels Oct 22, 2024
@anandamaryon1
Copy link
Collaborator

Your simpler approach makes sense to me, but given that GOV do the same, it'd be worth understanding why. Did you ask on xgov slack?

frankieroberto
frankieroberto previously approved these changes Nov 19, 2024
Copy link
Contributor

@frankieroberto frankieroberto left a comment

Choose a reason for hiding this comment

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

Looks a lot simpler to me! 🙌

I wonder if the .nhsuk-button--disabled class ought to be removed? I can’t see any valid use case for styling a button as disabled if it isn’t actually disabled, and if it is disabled, then the .nhsuk-button:disabled selector will style it anyway?

@frankieroberto
Copy link
Contributor

@paulrobertlloyd you’ll have to update/rebase from main to get the newly-split-up tests to run.

@paulrobertlloyd
Copy link
Contributor Author

Rebased with main.

Happy to remove .nhsuk-button--disabled. Right now this is still needed as the component allows for a disabled link, indicated using aria-disabled="true":

<a{{ commonAttributes | safe }} href="{{ params.href if params.href else '#' }}" draggable="false"{%- if params.disabled %} aria-disabled="true"{% endif %} role="button">

This is not addressable using the :disabled pseudo class, (though we could add [aria-disabled] if we wanted to remove the .nhsuk-button--disabled modifier).

Not sure if ‘disabled links’ is a markup pattern that should really be supported; GOV.UK Frontend only allows you to disable button buttons.

@paulrobertlloyd
Copy link
Contributor Author

@anandamaryon1 I asked on X-GOV Slack and this is likely a holdover from a previous design for disabled buttons which all used the same colour; I’m going to add a PR to simplify the disabled styles over there, too.

@frankieroberto
Copy link
Contributor

Hmm… links styled as disabled buttons… 😵‍💫

@paulrobertlloyd
Copy link
Contributor Author

More investigation… it’s complicated. govuk-fronted changes the pointer to not-allowed on hover, and thus cannot disable pointer-events which means hover styles need to be accounted for.

We disable pointer-events, which means we don’t need to account for hover (and active) styles. If we wanted to have the not-allowed pointer, we’d need a whole lot of CSS to revert the active plus disabled state… and that gets real ugly due to the box-shadow and faux-button press. Which is to say, I think this simplification works, in our case at least.

@anandamaryon1
Copy link
Collaborator

I'm happy with this, thanks @paulrobertlloyd

Links styled as disabled buttons doesn't seem good, I can't think of a legitimate use-case for them. Plus making actually disabled links is a whole messy thing: https://css-tricks.com/how-to-disable-links/

Is it gung-ho to just remove it?

@paulrobertlloyd
Copy link
Contributor Author

Is it gung-ho to just remove it?

I can well imagine some services using this anti-pattern; if we were to remove it in a minor release, links that should appear/behave disabled would potentially start ‘working’.

Let’s merge this fix in for the next minor release. I can create a PR to remove support for disabled links for the next major release.

Copy link
Collaborator

@anandamaryon1 anandamaryon1 left a comment

Choose a reason for hiding this comment

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

Nice work

@paulrobertlloyd paulrobertlloyd merged commit 4b7c7fc into main Nov 20, 2024
5 checks passed
@paulrobertlloyd paulrobertlloyd deleted the warning-button-disabled branch November 20, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buttons Code improvements Any changes that improve but do not alter the appearance of the site next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning button disabled styling
3 participants