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

Accordion as OL when needed (instead of always a UL) #16040

Closed
bdanbury opened this issue Mar 22, 2024 · 6 comments · Fixed by #16535
Closed

Accordion as OL when needed (instead of always a UL) #16040

bdanbury opened this issue Mar 22, 2024 · 6 comments · Fixed by #16535
Labels
afrohacks See https://ibm.biz/afrohacks-hackathon component: accordion hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. proposal: accepted This request has gone through triaging and we are accepting PR's against it. role: dev 🤖 status: help wanted 👐 type: enhancement 💡

Comments

@bdanbury
Copy link

bdanbury commented Mar 22, 2024

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @carbon/react@1.53.1 for the project I'm working on.

We are using Accordion to display a list of items which are best contained within an OL rather than a UL. We have patched v1.53.1 to allow wrapping items in either ul (default) or ol. Please consider carrying this work into the main branch.

I've attached the diff that solved my problem:

@carbon+react+1.53.1.patch

Copy link
Contributor

Thank you for submitting a feature request. Your proposal is open and will soon be triaged by the Carbon team.

If your proposal is accepted and the Carbon team has bandwidth they will take on the issue, or else request you or other volunteers from the community to work on this issue.

@tay1orjones
Copy link
Member

tay1orjones commented Apr 18, 2024

Thanks for attaching that patch file, it confirms there's no styling changes necessary to support this. Is the reasoning for using an OL in your case just improved semantics?

Docs on the website state that a list is necessary, I think ordered or unordered is probably fine.

Since accordions are typically grouped together, Carbon puts each button inside a list item in an unordered list, which provides additional context to screen reader users; where only one accordion is used, it should not be put in a list.

All in I think this is a reasonable thing to add, albeit low in priority. A PR for this would be appreciated.

@tay1orjones
Copy link
Member

Sorry, hit the wrong button 🙃

@tay1orjones tay1orjones reopened this Apr 18, 2024
@tay1orjones tay1orjones added status: help wanted 👐 proposal: accepted This request has gone through triaging and we are accepting PR's against it. needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. and removed status: needs triage 🕵️‍♀️ labels Apr 18, 2024
Copy link
Contributor

The Carbon team has accepted this proposal! Our team doesn't have the capacity to work on this now, so we are requesting community contributors. Please see the labels for roles that are needed. If you are willing to help out, comment below and we will get in touch!

@tay1orjones
Copy link
Member

tay1orjones commented May 2, 2024

For this we'd want to avoid an as prop. Instead this should be something like an ordered boolean prop that toggles ol instead of the ul default. Plus a new test case to ensure the ul is rendered when the prop is passed.

@tay1orjones tay1orjones added hacktoberfest See https://hacktoberfest.com/ afrohacks See https://ibm.biz/afrohacks-hackathon labels May 2, 2024
@Tresau
Copy link
Contributor

Tresau commented May 21, 2024

I can work on this @tay1orjones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
afrohacks See https://ibm.biz/afrohacks-hackathon component: accordion hacktoberfest See https://hacktoberfest.com/ needs: community contribution Due to roadmap and resource availability, we are looking for outside contributions on this issue. proposal: accepted This request has gone through triaging and we are accepting PR's against it. role: dev 🤖 status: help wanted 👐 type: enhancement 💡
Projects
Status: ✅ Done
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants