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

tabs: Add support for vertical orientation #486

Merged
merged 14 commits into from
Mar 29, 2020

Conversation

peterjmag
Copy link
Contributor

@peterjmag peterjmag commented Feb 21, 2020

Hey folks 👋

This PR adds a new orientation prop to the Tabs component and sets its default to "horizontal". The new prop has these effects:

  • Adds the aria-orientation attribute to the TabsList DOM element per the WAI-ARIA docs.
    • Since orientation has a default, that means the existing tab examples also get this new attribute.
  • When it's set to "vertical":
    • Changes the flex-direction of TabsList to stack tabs vertically.
    • Displays TabPanels to one side.
    • Allows the user to cycle through tabs with the up + down arrow keys (instead of the left + right arrows).
    • Allows the user to focus the active TabPanel with the right arrow key (or the left arrow key in RTL mode).

I did see this TODO:

* TODO: Consider `orientation` prop to account for keyboard behavior
* - horizontal-top
* - horizontal-bottm
* - vertical-left
* - vertical-right

This PR takes a slightly different approach, by limiting the options to just "horizontal" and "vertical" and then relying on RTL (or just the ordering of TabsList and TabPanels) for the rest. Let me know what you think!

I'm opening this as a draft PR, because I'm still trying to figure out what's going on with the tests, but I wanted to get some feedback on the approach so far.

Screenshots

Screen Shot 2020-02-21 at 13 54 22


Screen Shot 2020-02-21 at 13 54 38

PR template

Thank you for contributing to Reach UI! Please fill in this template before submitting your PR to help us process your request more quickly.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code (Compile and run).
  • Add or edit tests to reflect the change (Run with yarn test).
  • Add or edit Storybook examples to reflect the change (Run with yarn start).
  • Ensure formatting is consistent with the project's Prettier configuration.

This pull request:

  • Creates a new package
  • Fixes a bug in an existing package
  • Adds additional features/functionality to an existing package
  • Updates documentation or example code
  • Other

If creating a new package:

  • Make sure the new package directory contains each of the following, and that their structure/formatting mirrors other related examples in the project:
    • examples directory
    • src directory with an index.tsx entry file
    • At least one example file per feature introduced by the new package
    • Base styles in a style.css file (if needed by the new package)

@enforce-template-use
Copy link

Thank you for the pull request

orientation === "vertical"
? isRTL
? event.key === "ArrowLeft"
: event.key === "ArrowRight"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I'm just realizing: this doesn't take into account the relative order of TabsList and TabPanels. For instance:

<Tabs orientation="vertical">
  <TabPanels>
    <TabPanel>
      <h1>one!</h1>
      <p>Here's some example content.</p>
      <button>yo</button>
    </TabPanel>
    <TabPanel>
      <h1>two!</h1>
      <p>Here's some example content.</p>
    </TabPanel>
    <TabPanel>
      <h1>three!</h1>
      <p>Here's some example content.</p>
    </TabPanel>
  </TabPanels>

  <TabList>
    <Tab>One</Tab>
    <Tab>Two</Tab>
    <Tab>Three</Tab>
  </TabList>
</Tabs>

Assuming we're in LTR mode, TabPanels will show up on the left (which is correct), but the left arrow key won't focus the panel.

I'll think about how to address that, but let me know if you all have any ideas. Could actually be a good reason to make the options more explicit as @chancestrickland mentioned in his TODO i.e. horizontal-top, horizontal-bottom, etc.

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 took a shot at detecting the relative order (while still taking isRTL into account) in f07be1d.

@@ -143,7 +144,7 @@ function Example() {
onChange={index => setTabIndex(index)}
style={{
color: "white",
background: backgroundColor
background: backgroundColor,
Copy link
Contributor Author

@peterjmag peterjmag Feb 21, 2020

Choose a reason for hiding this comment

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

Oops, my local Prettier config might have interfered here. I'll revert these comma changes.

EDIT: Scratch that, looks like this is correct:

"trailingComma": "es5"

Still, I'm happy to revert the comma changes if you need me to!

@@ -91,6 +91,7 @@ Please see the [styling guide](/styling).
| [onChange](#tabs-onchange) | function | false |
| [defaultIndex](#tabs-defaultindex) | number | false |
| [index](#tabs-index) | number | false |
| [orientation](#tabs-orientation) | string | false |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is string enough for the type here? Or should I make it more specific, a la line 250?

packages/tabs/__tests__/tabs.test.tsx Outdated Show resolved Hide resolved
@chaance
Copy link
Member

chaance commented Feb 21, 2020

Looks like a solid start, thanks for the PR! I'll dig into this a bit next week and see what we can do.

@chaance chaance added the Type: Enhancement General improvements or suggestions label Feb 21, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 2, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8ce9450:

Sandbox Source
crazy-kapitsa-78i6x Configuration

@peterjmag peterjmag marked this pull request as ready for review March 5, 2020 17:25
@peterjmag
Copy link
Contributor Author

Hey @chancestrickland! The tests are now running through, so this PR is (finally) ready for a proper review whenever you get a chance ☺️

@chaance
Copy link
Member

chaance commented Mar 29, 2020

Thanks so much for this! Finally got around to reviewing, and it looks great. I made some adjustments to the API. Mainly, orientation will accept any of the following:

  • horizontal-start (default)
  • horizontal-end
  • vertical-start
  • vertical-end

The reason here is that I'd rather be explicit vs. checking children to see whether TabList or TabPanels comes first, which can break certain composition patterns. We could register refs similar to how we do for tab descendants, but a single prop change at the top along with added documentation to explain the distinctions seems simple enough for this purpose. App code can abstract this further if someone doesn't like it.

@chaance chaance added Type: Feature New feature request and removed Type: Enhancement General improvements or suggestions labels Mar 29, 2020
@chaance chaance merged commit a7ead6a into reach:master Mar 29, 2020
@chaance
Copy link
Member

chaance commented Mar 30, 2020

Slept on it, and now I'm having second thoughts. I'm going to seek some feedback on this approach. direction and orientation are two separate things at the end of the day, and we might be making too many assumptions.

@peterjmag
Copy link
Contributor Author

@chancestrickland No problem, and definitely no rush given everything that's going on right now. In any case, let me know if there's anything I can do to help gather feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants