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

fix(tabs-extended): change anchor-based tabs to button-based tabs #8365

Merged
merged 12 commits into from
Mar 8, 2022
Merged

fix(tabs-extended): change anchor-based tabs to button-based tabs #8365

merged 12 commits into from
Mar 8, 2022

Conversation

andy-blum
Copy link
Member

Related Ticket(s)

Fixes #8357

Description

Fixes an issue where iOS on tablet couldn't use tabs.

Changelog

Changed

  • changes <a> elements to <button> elements in tabs so iOS devices getting the desktop-style layout can navigate with voiceover

@andy-blum andy-blum requested a review from a team as a code owner February 23, 2022 20:39
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 23, 2022 •
edited
Loading

Deploy preview created for package Web Components HTML Test Application:
https://ibmdotcom-web-components-html-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: 03aea7087ab404ff3add7dee31135754430f8e6a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 23, 2022 •
edited
Loading

Deploy preview created for package Web Components (React wrapper):
https://ibmdotcom-web-components-react.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: 03aea7087ab404ff3add7dee31135754430f8e6a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 23, 2022 •
edited
Loading

Deploy preview created for package React:
https://ibmdotcom-react.s3.us-south.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: 03aea7087ab404ff3add7dee31135754430f8e6a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 23, 2022 •
edited
Loading

Deploy preview created for package Web Components:
https://ibmdotcom-webcomponents.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: 03aea7087ab404ff3add7dee31135754430f8e6a

@proeung proeung added 👀 eyes needed accessibility Has accessibility requirement Needs design approval PRs on feature requests and new components have to get design approval before merge. package: web components Work necessary for the IBM.com Library web components package labels Feb 23, 2022
@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 23, 2022 •
edited
Loading

Deploy preview created for package NextJS Test Application:
https://ibmdotcom-nextjs-test-upstream.s3.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: 03aea7087ab404ff3add7dee31135754430f8e6a

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 24, 2022 •
edited
Loading

Deploy preview created for package "Web Components (Codesandbox Examples)":
https://webcomponents-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: faa23f8889003a2ba65e6fc7658ebf4a6b5d768b

@ibmdotcom-bot
Copy link
Contributor

ibmdotcom-bot commented Feb 24, 2022 •
edited
Loading

Deploy preview created for package "React (Codesandbox Examples)":
https://react-codesandbox.s3-web.us-east.cloud-object-storage.appdomain.cloud/deploy-previews/8365/index.html

Built with commit: 5c25157f00c3882470307c3ed7baf1223cb2e043

@RichKummer
Copy link

Hey @andy-blum , I tested this on iOS and I'm seeing that we're able to navigate between tabs with VoiceOver on. I noticed that when a tab is focused and if the tab has a tooltip, he focus area includes the tooltip even if the tab is not active yet. Is this the expected behavior for tabs with tooltips?

While Tab 1 is active but with Tab 2 selected, you can see the focus dips below the tab itself
IMG_1192

Here Tab 2 is active and you can see that the focus is capturing the tooltip
IMG_1193

kennylam
kennylam reviewed Mar 2, 2022
View reviewed changes
Copy link
Member

@kennylam kennylam left a comment

Choose a reason for hiding this comment

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

Thanks @andy-blum! Just a few small things.

packages/styles/scss/components/tabs-extended/_tabs-extended.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/tabs-extended/_tabs-extended.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/tabs-extended/_tabs-extended.scss Outdated Show resolved Hide resolved
@RichKummer
Copy link

Hey @andy-blum , I'll wait to re-review until Kenny's suggested changes are in, but I checked again on tablet with VO and it looks like the change to the tab selection was resolved - yay!

I'll keep an eye out for the other changes and then run through it again then

@andy-blum andy-blum mentioned this pull request Mar 4, 2022
Merged
RichKummer
RichKummer approved these changes Mar 4, 2022
View reviewed changes
Copy link

@RichKummer RichKummer left a comment

Choose a reason for hiding this comment

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

Looks great @andy-blum , thanks for making the change!

@RichKummer RichKummer mentioned this pull request Mar 4, 2022
Closed
@RichKummer RichKummer removed the Needs design approval PRs on feature requests and new components have to get design approval before merge. label Mar 4, 2022
proeung
proeung suggested changes Mar 7, 2022
View reviewed changes
Copy link
Contributor

@proeung proeung left a comment

Choose a reason for hiding this comment

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

Looks good overall! I just have a few suggestions related to keeping with using carbon tokens for color and spacings.

packages/styles/scss/components/tabs-extended/_tabs-extended.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/tabs-extended/_tabs-extended.scss Outdated Show resolved Hide resolved
packages/styles/scss/components/tabs-extended/_tabs-extended.scss Outdated Show resolved Hide resolved
@mwanberg
Copy link
Contributor

mwanberg commented Mar 8, 2022

I think this will need to be merged before I can finish my related PR: #8394

proeung
proeung approved these changes Mar 8, 2022
View reviewed changes
Copy link
Contributor

@proeung proeung left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for applying my suggestions!

@proeung proeung added Ready to merge Label for the pull requests that are ready to merge and removed 🛠️ fix needed labels Mar 8, 2022
@kodiakhq kodiakhq bot merged commit 2225471 into carbon-design-system:main Mar 8, 2022
@andy-blum andy-blum deleted the fix/tabs-extended-iOS-VO branch March 9, 2022 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennylam kennylam kennylam left review comments

@RichKummer RichKummer RichKummer approved these changes

@proeung proeung proeung approved these changes

@IgnacioBecerra IgnacioBecerra Awaiting requested review from IgnacioBecerra IgnacioBecerra is a code owner automatically assigned from carbon-design-system/carbon-for-ibm-com-members

@annawen1 annawen1 Awaiting requested review from annawen1

Assignees
No one assigned
Labels
accessibility Has accessibility requirement package: web components Work necessary for the IBM.com Library web components package Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tabs Extended]: Tabs that aren't open by default are unusable on iOS with VoiceOver
6 participants
@andy-blum @ibmdotcom-bot @RichKummer @mwanberg @kennylam @proeung