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

Allow tab to be active but not selected #60573

Closed
wants to merge 1 commit into from
Closed

Conversation

jeryj
Copy link
Contributor

@jeryj jeryj commented Apr 8, 2024

More context in #52997 (comment)

What?

There is not a way to have the first tab be active (the one receiving focus) but not selected (have its tab pane visible). For the patterns inserter, I needed this behavior: #60257

Why?

Sometimes you don't want any tab pane selected by default. This was possible before, but the focus was set to the wrapping element, not the first tab.

How?

Adds a selectedOnMount prop that defaults to true to preserve the existing behavior. If you set selectedOnMount to false, it will set the first tab as the active one (receives focus) but not select it until an enter keypress on that tab.

Testing Instructions

Test the patterns inserter in #60257.

  • Open Block inserter (in post editor and in site editor)
  • Select Pattern Tab
  • Tab to patten categories panel
  • Focus should be on first tab
  • Use arrows to navigate between the patterns
  • Select a pattern
  • Tab into patterns panel
  • Shift + Tab out
  • Focus should be on the selected category
  • Arrow between patterns and repeat switching between categories

Testing Instructions for Keyboard

Screenshots or screencast

If defaultSelectedId is undefined, it will automatically set the first tab as selected.
@jeryj jeryj requested a review from ajitbohra as a code owner April 8, 2024 20:52
@jeryj jeryj self-assigned this Apr 8, 2024
Copy link

github-actions bot commented Apr 8, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jeryj <jeryj@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@jeryj jeryj added [Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement. labels Apr 8, 2024
@jeryj
Copy link
Contributor Author

jeryj commented Apr 8, 2024

@mirka - Here's an attempt at what we discussed in #52997 (comment). I'm not sure about the name of the new prop or if this is the best way to go about it. Curious to hear your and the @WordPress/gutenberg-components team opinions!

@jeryj jeryj requested review from mirka and chad1008 April 8, 2024 20:55
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Some new thoughts about our approach

I discussed this with @DaniGuardiola today, and he pointed out that the use case we have in the Patterns inserter is likely a better match with the Menu pattern rather than the Tabs pattern. Imagine a nested menu without the initial "Open menu" button, and that's basically the mechanics we want there. The visual styling is also much closer to a menu than a tabs pattern. The most persuasive point for me is that I don't think I've never seen a tabs UI without a default tab selected.

That said, I know that the original motiviation for you was to fix the keyboard navigation in the Patterns inserter. That can be done with Tabs, you already have a mostly working implementation, and it's not like the semantics are "wrong". So I'd be fine with shipping this Tabs approach as an incremental improvement. We can experiment with a Menu replacement later.

I'm not immediately sure if DropdownMenuV2 in its current state can be used for this use case, but ultimately it should, so this would be a good place to test the flexibility of our new composable components.

Thoughts on this PR

With that in mind, I don't think we'll have many cases of selectedTabId={ null }, nor do we really want to encourage it. Given the niche use case, I'm thinking we could just keep it simple and make "when activeId is undefined, fall it back to the first tab id" the baked-in behavior for our Tabs component. Thoughts?

@jeryj
Copy link
Contributor Author

jeryj commented Apr 10, 2024

I'm thinking we could just keep it simple and make "when activeId is undefined, fall it back to the first tab id" the baked-in behavior for our Tabs component.

I'm fine with this behavior. Fall it back to set an active id but not select it, right?

@mirka
Copy link
Member

mirka commented Apr 11, 2024

I'm thinking we could just keep it simple and make "when activeId is undefined, fall it back to the first tab id" the baked-in behavior for our Tabs component.

I'm fine with this behavior. Fall it back to set an active id but not select it, right?

Exactly 👍

@jeryj
Copy link
Contributor Author

jeryj commented Apr 11, 2024

Thanks for the feedback, @mirka! I'm closing this in favor of a fallback active tab: #60681

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Enhancement A suggestion for improvement.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants