Skip to content

Conversation

@jzempel
Copy link
Member

@jzempel jzempel commented Jun 3, 2024

Description

The previous verticalStyling catch-all function in StyledTabs was not well-structured, allowing component CSS to leak into child Tabs so that if any parent sets the isVertical prop, all children would be vertically styled regardless of their isVertical prop setting.

This PR eradicates the verticalStyling function by portioning all of its properties out to relevant components, conditionally and precisely applying CSS depending on the context. The updated styles follow Garden's API rules by utilizing sizeStyles and colorStyles accordingly.

Detail

closes #1817

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

Copy link
Contributor

@geotrev geotrev 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. Just the one comment.

One unrelated thing I noticed (present on main): trying to use arrow keys backward through tabs requires one extra arrow key press to loop from the first to last tab. Guessing that's in the tabs container logic though. We can track that separately but not sure if anyone else noticed.

'data-garden-version': PACKAGE_VERSION
})<IStyledTabsProps>`
display: block;
display: ${props => (props.isVertical ? 'table' : 'block')};
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondeirng if using flex might be preferable for handling flow. 🤔 display: table can impact the element's role for some screen readers (source). Granted, it's a small subset of SRs, so maybe not a huge deal?

Copy link
Contributor

@geotrev geotrev Jun 3, 2024

Choose a reason for hiding this comment

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

I also confirmed using a fake table demo in the link above that Chrome + JAWS treats CSS tables as actual tables (so long as they have more than one cell). 😬

Chrome + JAWS is kinda weird with CSS tables. maybe it's not a huge deal for us. The container isn't announced as a table so maybe it's OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geotrev all valid concerns for sure. For this PR I want to retain exact parity with the previous styling, since that's already been through a11y audit.

trying to use arrow keys backward through tabs requires one extra arrow key press to loop from the first to last tab

Looks like that might be related to the inclusion of a disabled tab in the Storybook example. Again, let's plan to address under a separate effort.

@jzempel jzempel merged commit 725cfb3 into main Jun 4, 2024
@jzempel jzempel deleted the jzempel/tabs-in-tabs branch June 4, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Nested Tabs isVertical inherit

4 participants