-
Notifications
You must be signed in to change notification settings - Fork 200
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
[Radio Button] Proposal (Editor's draft) #360
Conversation
Hi @una, @gregwhitworth and @levithomason, I am afraid this PR can become a little stale, but I wouldn't like to go very far before validating the main ideas addresses on it. What could we do to proceed? |
Thanks for filing this @leolopes - I've added @chrisdholt to do a review on this. Also, I may have missed it but did we discuss this anatomy and get resolution on it? |
@@ -0,0 +1,185 @@ | |||
--- | |||
menu: Proposals | |||
name: Radio Button (Editor's Draft) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The primary feedback I have here is whether we need to include button
in the element name. Radio itself is fairly common across component libraries and design systems, and it maps to the ARIA role and the "type" value of input currently in the platform. Button seems excessive and verbose without offering any additional value. Previously I think that is inline with what we resolved here, but I could be misremembering context.
Either way, I would propose Radio
and Radio Group
with <oui-radio>
and <oui-radio-group>
for elements.j
Thoughts @leolopes, @gregwhitworth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you @chrisdholt. Radio
is representative and concise enough, and will not clash with the idea of "buttons" in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the documents still reference radio-button
rather than radio
as discussed here. Per this thread, should these instances be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change anything at first because the rest of the discussion did not advance. But as soon as I get a clear request from the reviewers, I will change it.
@leolopes this is great! @gregwhitworth I don't think we discussed and resolved. There are a couple super interesting things I think we should discuss though and get resolution on so we can move this forward! |
Hi @gregwhitworth, how are you? We discussed during a meeting the possibility of having the component either be bound together by tag nesting or by attributes. Thus I proposed both ways in this draft, so the actual proposal can be discussed further. I'll be answering @chrisdholt on each of his points. |
Hello reviewers! |
@@ -0,0 +1,185 @@ | |||
--- | |||
menu: Proposals | |||
name: Radio Button (Editor's Draft) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the documents still reference radio-button
rather than radio
as discussed here. Per this thread, should these instances be updated?
|
||
## Radio Button Design | ||
|
||
This section relates to a single `<oui-radio-button>`. Because it cannot stand on its own, without being part of and either explicit or implicit `<oui-radio-button-group>`, it does not have attributes such `required` or `readonly`, those being attributes of the group. A single `required` option is a group makes the other choices pointless, while a `readonly` option makes itself pointles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: the allowance of the required attribute on a single radio - this makes sense in the context of a radiogroup
where the only children of the group would be expected to be radio buttons, and at the parent level the requiredness of the grouping could be declared.
However, current radio buttons in HTML have greater flexibility in where they can be placed / associated with each other, and can still be communicated as required with this flexibility.
for example:
<label><input type=radio name=r required> Choice 1</label>
Other information or form fields, or both, associated with making choice 1.
...
<label><input type=radio name=r> Choice 2</label>
Other information or form fields, or both, associated with making choice 2.
This would need to be thought out more in how to communicate the required nature of radios that don't neatly fit into a grouping (see my next comment for more on that) or a merging of what HTML presently allows (putting the required attribute on a radio button) and what the ARIA radio/radiogroup roles expect (that if you're using radios - particularly when a choice is required - they are part of a radiogroup, is more limiting than HTML)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the code you used as an example mean that, by placing required
in one of the options, the group is required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. the implicit grouping becomes required and you cannot submit a form unless one of the radios is checked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, I was not aware of it.
@leolopes friendly ping on this, do you intend on taking this further? |
Hi @gregwhitworth , I do, I just have been overwhelmed by work and I thought the proposal didn't get much traction so I didn't prioritize this. But I'll try to reserve some time this week and fix conflicts and make some of the requested changes. |
…nts would make their group required or readonly.
@gregwhitworth , I was trying to resolve the conflicts and now I noticed that the website was rewriten wit Astro in place of Gatsby. I'll have to learn it and make a lot of adjustments so my content will be able to run on the new structure. |
…l/radio-button # Conflicts: # research/src/pages/radio-button.research.mdx # site/public/components/radio-anatomy.js # site/public/components/radio-group-anatomy.js # site/src/pages/components/radio-button.research.mdx # site/src/pages/components/radio/radio.proposal.mdx # site/src/pages/components/radio/radio.research.mdx
There seems to be an error on a file that was not modified by me. I'm not sure how it can be, because I pulled directly from I can try to fix the file and push again later. |
@andrico1234 can you take a look at this given the error that @leolopes is seeing? |
@gregwhitworth and @andrico1234 I found out what was happening: Husky was messing up with a commented section on the popover.research.explainer.mdx file. I had to bypass the linting proccess on my last commit so I wouldn't run into a syntax error again. I think either the linter should be reconfigured or the commented section should be removed. Anyway, the PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed thoroughly the definitions of the items nor their anatomy's but there is a key change that I'd like to request.
I would love for your proposal to follow the shape of the Popup API where it makes sense. Some key aspects I'd like:
- Background: Get into why there is a need for both Radio and Radio Group. you cover this generally but I'd love for the problem to be up front in the explainer whereas right now it's spread throughout the research document.
- Problem statement: You may briefly hit on this in the background but enumerate each of the problems that not having these controls currently present to end users. Again, you have this content just move it up here.
- Goals: This is where you'll enumerate the goals which aim to solve the problem statements
- Radio Element: For now let's remove the API section and lead with the anatomy and definitions. If necessary, don't be afraid to expound on key details of various parts
- Radio Group: Same as radio element
- Other solutions considered:
- Why not just use
<select>
or<selectmenu>
?
- Why not just use
Thank you again for doing this work; and while the above may seem daunting it's more a shuffle of the content that you have as we've realized as Open UI has matured that we should primarily author explainers which should try and read as a single document even though there may be supporting content; but someone shouldn't have to read that content to fully understand the proposal and the problem that it's solving.
|
||
## Introduction | ||
|
||
_(For a lengthier discussion, on which this page was based, please refer to the [original radio button issue](https://github.com/openui/open-ui/issues/251))_ | ||
_(For a lengthier discussion, on which this page was based, please refer to the [original radio issue](https://github.com/WICG/open-ui/issues/251))_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove this or just make this a bit more conversational, but prior to doing this - please see my overall comment regarding the editor's draft and research.
|
||
- as a standalone component, that could be hosted on a “**radio button group**” component; | ||
- as an invariably non-independent part of a “**radio button group**” component (just like `<option>` tags are dependent on `<select>` tags). | ||
- as a standalone component, that could be hosted on a “**radio group**” component; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this be within rather that on ?
|
||
The two approaches seem very similar at first, but the former seems to indicate that single “radio buttons” could exist, to probably indicate a binary state (on or off). The latter, however, shifts the scope up to the “radio button group”, which, although nonexistent as an HTML element, is an important concept for the W3 specification: | ||
The two approaches seem very similar at first, but the former seems to indicate that single “radios” could exist, to probably indicate a binary state (on or off). The latter, however, shifts the scope up to the “radio group”, which, although nonexistent as an HTML element, is an important concept for the W3 specification: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove probably and be a bit more emphatic on your takeaways from the research. If you don't have a strong position then make it an open question.
|
||
## Discussion | ||
|
||
At a point during the research, the scope of this study os shifted from the “radio button” to the “radio button group”, because there is no particular strong use case for a standalone “radio button”, and also because this use case is against the W3 Specification, the actual component to be considered is the “radio button group”, being composed of two or more radio buttons. | ||
At a point during the research, the scope of this study os shifted from the “radio” to the “radio group”, because there is no particular strong use case for a standalone “radio”, and also because this use case is against the W3 Specification, the actual component to be considered is the “radio group”, being composed of two or more radios. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove os as "the scope of this study shifted" is sufficient
there is no strong usecase for a radio
I disagree with this stance as I've heard the same statement for checkbox but can make an argument for it
|
||
## Discussion | ||
|
||
At a point during the research, the scope of this study os shifted from the “radio button” to the “radio button group”, because there is no particular strong use case for a standalone “radio button”, and also because this use case is against the W3 Specification, the actual component to be considered is the “radio button group”, being composed of two or more radio buttons. | ||
At a point during the research, the scope of this study os shifted from the “radio” to the “radio group”, because there is no particular strong use case for a standalone “radio”, and also because this use case is against the W3 Specification, the actual component to be considered is the “radio group”, being composed of two or more radios. | ||
|
||
Also based on this fact and on the current state of the design systems, a more final anatomy was proposed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow this sentence.
|
||
## Discussion | ||
|
||
At a point during the research, the scope of this study os shifted from the “radio button” to the “radio button group”, because there is no particular strong use case for a standalone “radio button”, and also because this use case is against the W3 Specification, the actual component to be considered is the “radio button group”, being composed of two or more radio buttons. | ||
At a point during the research, the scope of this study os shifted from the “radio” to the “radio group”, because there is no particular strong use case for a standalone “radio”, and also because this use case is against the W3 Specification, the actual component to be considered is the “radio group”, being composed of two or more radios. | ||
|
||
Also based on this fact and on the current state of the design systems, a more final anatomy was proposed. | ||
|
||
### Proposed Anatomy - by Tag Hierarchy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove proposed anatomy and just make it "Anatomy" as every spec in Open UI is effectively a proposal and as such makes this title redundant
|
||
Also based on this fact and on the current state of the design systems, a more final anatomy was proposed. | ||
|
||
### Proposed Anatomy - by Tag Hierarchy | ||
|
||
This section shows the proposed visual constituents for the _Radio Button Group_, which is here considered to be the actual component, while the radio buttons are a part of the group. | ||
This section shows the proposed visual constituents for the _Radio Group_, which is here considered to be the actual component, while the radios are a part of the group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as with the title, remove proposed
- Group Label (functions as a label for the whole group) | ||
- Required Indicator | ||
- Radio Button (at least two required) | ||
- Radio (at least two required) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while I like this break down, we should have some text that explains why two are required.
- and so on... | ||
- and so on... | ||
|
||
#### The problem | ||
|
||
The above anatomy is regular enough to be assembled in a way similar to (but more complex than) a `<select>`. Attributes set on the group would allow the developer to set the whole group as "disabled", "readonly", vertically or horizontally laid out, types of display (like buttons, radio buttons, combobox, etc), and attributes set on the radio buttons would set their specific states, as we do with `<option>` tags. It is important to understand that the function of radio buttons is almost the same as a `<select>` tag, and this is why the comparison makes sense. | ||
The above anatomy is regular enough to be assembled in a way similar to (but more complex than) a `<select>`. Attributes set on the group would allow the developer to set the whole group as "disabled", "readonly", vertically or horizontally laid out, types of display (like buttons, radios, combobox, etc), and attributes set on the radios would set their specific states, as we do with `<option>` tags. It is important to understand that the function of radios is almost the same as a `<select>` tag, and this is why the comparison makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't bring <select>
at this stage. Possibly consider a section at the end with something like, "Other solutions considered"
That's a bit of work but I can manage it 😉 . Just not sure when I'll get back to it, but not as long as it took before. |
This can be re-opened if someone wants to pick it up. It hasn't had progress for over a year now so closing it out. |
@gregwhitworth I will eventually get back to it, maybe during vacation time. |
@leolopes it's alright; you can always re-open this. Life happens and we appreciate any contributions that can be added. |
Related to: #251
This PR is still a draft for the radio button proposal. I managed to create two components, one for the radio button and other for the radio button group, and the two would be all it takes to create both a rigid and a flexible layout.
The idea expounded on the proposal, but the gist is that we can either create a group with options inside of it, or a group with options outside of it, that are then bound to the group by a new attribute.
By leveraging slot defaults/fallbacks and proper CSS, we can make basically any layout use case.
===
I would like to validate the basic proposal with the community before developing it too much (you can see I didn't provide much information yet on accessibility, behavior, etc). If this is a valid proposal, I'll carry on.
===
Also, I'm unsure whether I used the "slot logic" correctly... In the group structure, you can see that I put a
div
inside thechoices
slot, but maybe the slot should be a direct parent of the radio buttons... If the developer so chooses, then they could fill the slot with adiv
and then the options. I'm not sure and I'd like your opinion as well.