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

Confused by the documentation for radios 'conditional' #1903

Closed
joelanman opened this issue Aug 6, 2020 · 7 comments · Fixed by #3552
Closed

Confused by the documentation for radios 'conditional' #1903

joelanman opened this issue Aug 6, 2020 · 7 comments · Fixed by #3552
Assignees
Labels
documentation User requests new documentation or improvements to existing documentation 🕔 hours A well understood issue which we expect to take less than a day to resolve. radios
Milestone

Comments

@joelanman
Copy link
Contributor

joelanman commented Aug 6, 2020

I was trying to use the conditional option on radios.

I saw that the example used

conditional:{
    html:"<p> ...

I didn't need html, just a sentence so I thought I could use text instead of html as you can in other places in Frontend. However, I tried it and it didn't work - there's no text option here.

When I re-read the docs, I misread and thought the 'conditional' option, being a string, could just accept text.

However, I misread it, that doesn't work - the docs actually say:

conditional | string | If true, content provided will be revealed when the item is checked.

But I don't think this is correct? In the examples, conditional is not a string, it's an object with only one property - html

I wonder if this might be better as two options:

conditionalHTML and conditionalText

This would avoid another object, and be more consistent with the use of HTML and text elsewhere?

@hannalaakso hannalaakso added the awaiting triage Needs triaging by team label Aug 6, 2020
@vanitabarrett
Copy link
Contributor

vanitabarrett commented Aug 7, 2020

@joelanman Can you provide a link to that in the docs, as that looks like it needs correcting? It should read:

conditional | boolean | If true, content provided will be revealed when the item is checked.
conditional.html | string | Provide content for the conditional reveal.

(which I can see on https://design-system.service.gov.uk/components/checkboxes/)

Good point about being able to provide a text option though, that definitely seems to better match what we do elsewhere.

@joelanman
Copy link
Contributor Author

It was radios I was looking at:

https://design-system.service.gov.uk/components/radios/

I would say even boolean is a bit confusing, as the examples don't use a boolean, they use an object

@36degrees 36degrees added documentation User requests new documentation or improvements to existing documentation radios labels Aug 13, 2020
@kellylee-gds kellylee-gds added 🕔 hours A well understood issue which we expect to take less than a day to resolve. and removed awaiting triage Needs triaging by team labels Aug 17, 2020
@36degrees
Copy link
Contributor

For now we've agreed just to remove the conditional entry from the macro options for radios and checkboxes, which will make it consistent to the way the accordion works and is documented (apart from not supporting conditional.text).

@mogusbi-motech
Copy link

The documentation around this is still a bit off as it still suggests setting conditional with a text value would work, which it definitely doesn't judging by the code below:

https://github.com/alphagov/govuk-frontend/blob/master/src/govuk/components/radios/template.njk#L14-L19

Documentation reference: https://design-system.service.gov.uk/components/radios/

image

@EoinShaughnessy
Copy link
Contributor

EoinShaughnessy commented Aug 19, 2021

The macro options for radios and checkboxes still display the conditional parameter. Do we need to remove these instances?

@EoinShaughnessy EoinShaughnessy added documentation User requests new documentation or improvements to existing documentation and removed documentation User requests new documentation or improvements to existing documentation labels Dec 23, 2021
@joelanman
Copy link
Contributor Author

joelanman commented Jul 15, 2022

just come across this again in my work - conditional is still listed as a string option, and as boolean in checkboxes, both should probably be removed

@36degrees 36degrees self-assigned this Apr 26, 2023
@36degrees 36degrees moved this from Backlog 🗄 to Needs review 🔍 in GOV.UK Design System cycle board Apr 26, 2023
@36degrees 36degrees added this to the v5.0 milestone Apr 26, 2023
@querkmachine querkmachine moved this from Needs review 🔍 to Ready to release 🚀 in GOV.UK Design System cycle board Apr 26, 2023
@joelanman
Copy link
Contributor Author

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation User requests new documentation or improvements to existing documentation 🕔 hours A well understood issue which we expect to take less than a day to resolve. radios
Projects
Development

Successfully merging a pull request may close this issue.

7 participants