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

[Radio Button] Proposal (Editor's draft) #360

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

leolopes
Copy link
Contributor

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 the choices 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 a div and then the options. I'm not sure and I'd like your opinion as well.

@leolopes leolopes changed the title Proposal/radio button (Editor's draft) [Radio Button] Proposal (Editor's draft) Jun 24, 2021
@leolopes leolopes mentioned this pull request Jun 30, 2021
@leolopes
Copy link
Contributor Author

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.

@gregwhitworth
Copy link
Member

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)
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

@leolopes leolopes Apr 18, 2022

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.

@chrisdholt
Copy link
Collaborator

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?

@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!

@leolopes
Copy link
Contributor Author

leolopes commented Oct 6, 2021

Also, I may have missed it but did we discuss this anatomy and get resolution on it?

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.

@leolopes
Copy link
Contributor Author

Hello reviewers!
Can we proceed with this PR or maybe it is too stale to be merged at this point?

research/src/pages/radio-button/radio-button.proposal.mdx Outdated Show resolved Hide resolved
@@ -0,0 +1,185 @@
---
menu: Proposals
name: Radio Button (Editor's Draft)
Copy link
Collaborator

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.
Copy link
Collaborator

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)

Copy link
Contributor Author

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?

Copy link
Collaborator

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

Copy link
Contributor Author

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.

research/src/pages/radio-button/radio-button.proposal.mdx Outdated Show resolved Hide resolved
@gregwhitworth
Copy link
Member

@leolopes friendly ping on this, do you intend on taking this further?

@gregwhitworth gregwhitworth marked this pull request as draft March 11, 2023 02:09
@leolopes
Copy link
Contributor Author

@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.

@leolopes
Copy link
Contributor Author

@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
@leolopes
Copy link
Contributor Author

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 master.

I can try to fix the file and push again later.

@gregwhitworth
Copy link
Member

@andrico1234 can you take a look at this given the error that @leolopes is seeing?

@leolopes
Copy link
Contributor Author

@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.

@leolopes leolopes marked this pull request as ready for review March 26, 2023 18:59
Copy link
Member

@gregwhitworth gregwhitworth left a 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>?

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))_
Copy link
Member

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;
Copy link
Member

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:
Copy link
Member

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.
Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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.
Copy link
Member

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)
Copy link
Member

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.
Copy link
Member

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"

@leolopes
Copy link
Contributor Author

...while the above may seem daunting it's more a shuffle of the content...
@gregwhitworth

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.
Thanks!

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.

None yet

4 participants