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

feat(theme-classic): allow passing tab label and default value through TabItem #5442

Merged
merged 8 commits into from
Sep 1, 2021
Merged

feat(theme-classic): allow passing tab label and default value through TabItem #5442

merged 8 commits into from
Sep 1, 2021

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Aug 30, 2021 •
edited
Loading

@Josh-Cena
Implement feature
061f8d5 
Signed-off-by: Josh-Cena <[email protected]>
@Josh-Cena
Dogfood edge case
c5f59f9 
Signed-off-by: Josh-Cena <[email protected]>
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 30, 2021
@netlify
Copy link

netlify bot commented Aug 30, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 061f8d5

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/612cb852ea12c1000741200d

😎 Browse the preview: https://deploy-preview-5442--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Aug 30, 2021 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 98
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5442--docusaurus-2.netlify.app/

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 86
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5442--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena changed the title feat(theme-classic): allow passing tab label and default value through TabItem children feat(theme-classic): allow passing tab label and default value through TabItem Aug 30, 2021
@netlify
Copy link

netlify bot commented Aug 30, 2021 •
edited
Loading

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 4d125dd

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/612f5403d27cb70007168874

😎 Browse the preview: https://deploy-preview-5442--docusaurus-2.netlify.app

@Josh-Cena
Correct keydown handling
a971e92 
Signed-off-by: Josh-Cena <[email protected]>
@Josh-Cena Josh-Cena mentioned this pull request Sep 1, 2021
Merged
slorber
slorber approved these changes Sep 1, 2021
View reviewed changes
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Just polished/simplified a bit the doc

IMHO it could be useful to provide a way to use Tabs without value. If TabItem labels are distinct (generally the case) they can be used as TabItem values too, making usage even simpler

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Sep 1, 2021 •
edited
Loading

IMHO it could be useful to provide a way to use Tabs without value. If TabItem labels are distinct (generally the case) they can be used as TabItem values too, making usage even simpler

You can already use tabs without label and just value (demonstrated by the last example)🤔 That's much more rigorous than only label without value IMO

@Josh-Cena
Copy link
Collaborator Author

As you can see there's still some syntactic noise when you declare labels in the Tabs props, because the implementation doesn't assign those to child tab items automatically. Not sure if it's worth fixing

@slorber
Copy link
Collaborator

slorber commented Sep 1, 2021

You can already use tabs without label and just value (demonstrated by the last example)🤔 That's much more rigorous than only label without value IMO

I think we could support both, one will be a fallback to the other and you can provide only one of those.

But it's not a big deal, this PR is fine to merge now thanks

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Sep 1, 2021
@slorber slorber merged commit 5f003bc into facebook:main Sep 1, 2021
@Josh-Cena Josh-Cena deleted the tab-children branch September 1, 2021 10:45
@slorber slorber mentioned this pull request Oct 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lex111 lex111 lex111 left review comments

@slorber slorber slorber approved these changes

Assignees
No one assigned
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Tabs collect values from TabItem children
4 participants
@Josh-Cena @slorber @lex111 @facebook-github-bot