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

feat(tabs): add new tabs components #2002

Open
wants to merge 5 commits into
base: next
Choose a base branch
from

Conversation

sofiushko
Copy link
Contributor

No description provided.

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Visual Tests Report is ready.

@sofiushko sofiushko changed the title feat(tabs): add new tabs component feat(tabs): add new tabs components Dec 23, 2024
@ogonkov
Copy link
Contributor

ogonkov commented Dec 28, 2024

All this changes for tabs?

@korvin89
Copy link
Contributor

korvin89 commented Jan 9, 2025

@sofiushko Hey! Could you rebase your branch?

@sofiushko sofiushko force-pushed the tabs2 branch 5 times, most recently from cb3e9d8 to 4921035 Compare January 14, 2025 16:39
@sofiushko
Copy link
Contributor Author

@sofiushko Hey! Could you rebase your branch?

Done
Is visual test [DefinitionList › smoke [direction: horizontal]] stable?

@sofiushko
Copy link
Contributor Author

@amje, @korvin89
Hey, rebased this branch again

Copy link
Contributor

@korvin89 korvin89 left a comment

Choose a reason for hiding this comment

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

There is a w3c design pattern for tabs. For now there are some differences between this pattern and current code behaviour:

  • Enter key doesn't activate tab
  • Arrows don't move focus within tabs
  • I'm not quite sure about moving focus between tabs by Tab key pressing

src/components/lab/Tabs/README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
import {TabContext} from './contexts/TabContext';

export type TabProviderProps = React.PropsWithChildren<{
value: string | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not value?: string?

src/components/tabs/TabList.tsx Show resolved Hide resolved
}
}

&-title {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is one &-title above, do we actually need to repeat it here?


return (
<div
{...filterDOMProps(props, {labelable: true})}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it assumed that users always should set aria-labelledby by themselves?

@sofiushko
Copy link
Contributor Author

sofiushko commented Jan 23, 2025

There is a w3c design pattern for tabs. For now there are some differences between this pattern and current code behaviour:

I relied on gravity-ui/rfc#16
There is a fundamental difference between these components behaviour.

  • Enter key doesn't activate tab

It`s ok, we can do this

  • Arrows don't move focus within tabs

it`s not ok. If we want this behaviour, we need to make tabs component uncontrolled. It goes against the issue. Maybe we should change the interface to make it possible?

  • I'm not quite sure about moving focus between tabs by Tab key pressing

I repeated the current component in this moment

@amje
Copy link
Contributor

amje commented Jan 23, 2025

@sofiushko I agree that focus behaviour should follow w3c pattern (arrow keys navigation instead of tabs). But I don't understand why we should change the API to make it possible

@sofiushko
Copy link
Contributor Author

@sofiushko I agree that focus behaviour should follow w3c pattern (arrow keys navigation instead of tabs). But I don't understand why we should change the API to make it possible

So sorry
I think you want to activate the newly tab on Arrow Click, according to
Left Arrow: moves focus to the previous tab. If focus is on the first tab, moves focus to the last tab. Optionally, activates the newly focused tab
If only focus - it possible, i will think about realization =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants