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.

Sign up for GitHub

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

Jump to bottom

Added examples for Radios in List Group #36644

Merged
merged 4 commits into from Jun 30, 2022
Merged

Added examples for Radios in List Group #36644

merged 4 commits into from Jun 30, 2022

Conversation

nkdas91
Copy link
Contributor

@nkdas91 nkdas91 commented Jun 29, 2022 •
edited by julien-deramond

@julien-deramond julien-deramond requested a review from a team June 29, 2022 16:13
julien-deramond
julien-deramond reviewed Jun 29, 2022
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for this PR @nkdas91. Just few comments based on my opinion only to lighten this part of the documentation (except the one regarding the checked attribute).

site/content/docs/5.2/components/list-group.md Outdated Show resolved Hide resolved
site/content/docs/5.2/components/list-group.md Outdated Show resolved Hide resolved
site/content/docs/5.2/components/list-group.md Show resolved Hide resolved
ffoodd
ffoodd requested changes Jun 29, 2022
View reviewed changes
site/content/docs/5.2/components/list-group.md Outdated Show resolved Hide resolved
site/content/docs/5.2/components/list-group.md Outdated Show resolved Hide resolved
@nkdas91
Copy link
Contributor Author

nkdas91 commented Jun 30, 2022

  • Reduced the number of checkboxes and radios to 3.

  • Used <label> and dropped aria-label.
    There is a statement: "You can use them without labels, but please remember to include an aria-label attribute and value for accessibility."
    Should we remove this?

  • Checked the first radio.

  • Used .stretched-link on <label>s to cover the whole list group item.

  • Removed radios from the stretched link example.

Thanks!

ffoodd
ffoodd approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Thanks ! LGTM now.

Regarding the sentence, I'm not sure. IMHO such way should be discouraged but it'd need to be tackles globally in docs and exemples, so don't bother in this PR.

@julien-deramond any thought?

julien-deramond
julien-deramond requested changes Jun 30, 2022
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Small change to do reported by our GitHub workflow regarding the docs and after that LGTM 🚀

site/content/docs/5.2/components/list-group.md Outdated Show resolved Hide resolved
site/content/docs/5.2/components/list-group.md Outdated Show resolved Hide resolved
@julien-deramond
Copy link
Member

Regarding the sentence, I'm not sure. IMHO such way should be discouraged but it'd need to be tackles globally in docs and exemples, so don't bother in this PR.

Agreed. I'll try to look at it globally in the docs 👌

@julien-deramond julien-deramond self-requested a review June 30, 2022 06:13
julien-deramond
julien-deramond approved these changes Jun 30, 2022
View reviewed changes
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

I managed to modify the <div class="list-group"> -> <ul class="list-group"> directly from the interface. Thanks for the PR @nkdas91 :)

@GeoSot GeoSot added this to In progress in v5.2.0-stable via automation Jun 30, 2022
nkdas91 and others added 4 commits June 30, 2022 22:54
@nkdas91 @GeoSot
Add examples for Radios in List Group
8da1c37
@nkdas91 @GeoSot
Reduce the number of checkboxes and radios to 3 for consistency
2d5740b 
Drop aria-label on inputs and use label
Use class stretched-link on labels to cover the whole list group item
Check the first radio by default
Remove radios from streched link examples
@julien-deramond @GeoSot
Update site/content/docs/5.2/components/list-group.md
98f260d
@julien-deramond @GeoSot
Update site/content/docs/5.2/components/list-group.md
effda5d
@GeoSot GeoSot merged commit f2692b1 into twbs:main Jun 30, 2022
v5.2.0-stable automation moved this from In progress to Done Jun 30, 2022
@nkdas91 nkdas91 deleted the radio-list branch July 1, 2022 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffoodd ffoodd ffoodd approved these changes

@julien-deramond julien-deramond julien-deramond approved these changes

Assignees
No one assigned
Labels
docs v5
Projects
No open projects
v5.2.0-stable
  
Done
v5.2.0-stable
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The "Radio" example is missing from the "List Group". Suggest a new feature
4 participants
@nkdas91 @julien-deramond @ffoodd @GeoSot