Skip to content
This repository has been archived by the owner on Nov 18, 2021. It is now read-only.

Hide native radios and checkboxes while leaving them accessible to dragons #352

Merged
merged 2 commits into from Jun 25, 2020

Conversation

chee
Copy link
Member

@chee chee commented Jun 24, 2020

this should fix these:
https://financialtimes.atlassian.net/browse/NOPS-235
https://financialtimes.atlassian.net/secure/RapidBoard.jspa?rapidView=1108&projectKey=ACC&modal=detail&selectedIssue=ACC-423

and this Financial-Times/n-feedback#88

tested in:

Dragon on IE11 on Windows 10
Firefox on Linux

hopefully a kind robot can show us some visual regression tests from other browsers so we know we haven't made anything worse?

Also worth testing it has the same screenreader experience as the previous version

npm i -g origami-build-tools
git clone git@github.com:financial-times/o-forms
cd o-forms
obt install
obt dev

and try out http://localhost:8999/checkboxes and http://localhost:8999/radio-round

@chee chee requested a review from a team as a code owner June 24, 2020 15:14
@chee chee added the percy This will tell the percy github action to run label Jun 24, 2020
@github-actions github-actions bot added the component Relates to an Origami component label Jun 24, 2020
@origamiserviceuser origamiserviceuser added this to incoming in Origami ✨ Jun 24, 2020
@origamiserviceuser
Copy link

👋 Percy has finished running the visual regression testing!

@origamiserviceuser origamiserviceuser removed component Relates to an Origami component percy This will tell the percy github action to run labels Jun 24, 2020
@chee
Copy link
Member Author

chee commented Jun 24, 2020

that hodgehog is taking 144 screenshots

@chee
Copy link
Member Author

chee commented Jun 24, 2020

🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔 🦔

@chee
Copy link
Member Author

chee commented Jun 24, 2020

um. according to the hedgehog everything got slightly bigger

@chee chee added the release:patch Add to a PR to trigger a PATCH version bump when merged label Jun 24, 2020
@NickColley
Copy link

NickColley commented Jun 24, 2020

I dug out GOV.UK Elements (the thing that predates GOV.UK Design System) to see why the Design System custom inputs use this method of opacity instead of the regular visually hidden class and found they ran into the same issue:

@chee
Copy link
Member Author

chee commented Jun 24, 2020

I dug out GOV.UK Elements (the thing that predates GOV.UK Design System) to see why the Design System custom inputs use this method of opacity instead of the regular visually hidden class and found they ran into the same issue:

* [opacity change](https://github.com/alphagov/govuk_elements/commit/f1fdcc45516c91e124e5476c12d7ff0994dd9374)

* [z-index: 1 change](https://github.com/alphagov/govuk_elements/commit/7190226a366570028d3a54bbb0a0567afe1e50df)

nice! excellent prior art

Copy link

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Since now the real input is in front of the custom input the cursor is back to the regular pointer instead of the hand.

@github-actions github-actions bot added the component Relates to an Origami component label Jun 24, 2020
Copy link
Contributor

@thurstontye thurstontye left a comment

Choose a reason for hiding this comment

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

👍 nice to have the comments re dragon on this too.

@NickColley
Copy link

In GOV.UK's work they reset the margin to zero, this seems to be done for us using normalize. Which is an dependency so should be fine:

https://github.com/Financial-Times/o-forms/blob/master/bower.json#L13

Copy link

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

I've tested that saying "Click Stacked 1" and "Click Default 1" works in both examples in Dragon.

@NickColley
Copy link

Non-blocking: May be able to remove the cursor: pointer from the pseudo element now since it's not doing anything? 🤔

@chee
Copy link
Member Author

chee commented Jun 24, 2020

nice, thank you so much @NickColley !

i'll merge this tomorrow morning.

if i could still get ft.com things to work on my laptop then i'd even try it in the context of some apps, but

@NickColley
Copy link

@chee if you can explain what you're aiming to do and some of the steps I may be able to do that bit. Let me know.

@chee
Copy link
Member Author

chee commented Jun 24, 2020

@NickColley installing this branch of o-forms in the front page app and checking out the n-feedback form, or in myft and testing out the problematic radio buttons

@taraojo
Copy link

taraojo commented Jun 24, 2020

This is great, thanks @chee 🎉
➕ 1️⃣ for the Dragon comment too

@chee chee requested a review from taraojo June 24, 2020 18:05
Copy link

@taraojo taraojo left a comment

Choose a reason for hiding this comment

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

💃

@chee chee merged commit 8caafe2 into master Jun 25, 2020
Origami ✨ automation moved this from incoming to complete Jun 25, 2020
@chee chee deleted the dragons branch June 25, 2020 12:45
@origamiserviceuser
Copy link

🎉 This PR is included in version v8.3.9 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component Relates to an Origami component release:patch Add to a PR to trigger a PATCH version bump when merged
Projects
Origami ✨
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants