Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding collapsible Tabs #1145
Adding collapsible Tabs #1145
Changes from all commits
a43777b
7b53e75
9e5e166
6c6a183
f258982
f50a629
2978f82
8600724
7824f16
f80b6f7
2573fef
427bc9a
f8acf57
ca49758
fc02b87
3610774
cd1c806
75395ce
80977cd
30cfdba
b9578ae
1eb399c
b8e7d36
6cba460
b708909
2950593
1cf80c4
58ec201
3d8f7ce
43767cc
8e92911
ef50b99
613f666
7c8d0cb
8195b11
558a968
96645c4
a47a95e
55c6764
62215a7
eac285a
7278408
40c87bb
21be5ae
67110ee
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this was moved to the react-spectrum package, why was that? just curiosity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it would be nice to allow users to customize their own tab fallback strategy when a tab isn't selected or the selected tab is deleted from the child list so I pulled it out to the spectrum package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think we need to at least have a default. I think it's weird that you'd need to implement this yourself when it will be most common for the first tab to be selected on mount. If you wish to control it some other way, you can use the controlled props. IMO this logic should be in stately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on this approach for propagating focus ring overrides?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm wondering if button should be the default slot name....
is it ok to conflict? https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/button/src/Button.tsx#L32
fortunately they can be overridden, but i wonder if it's counterintuitive or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extent of my logic was that they should share the same slot name since they are both buttons and may end up being used in similar slot positions lol. Happy to change it to
fieldbutton
or somethingThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i had that thought as well, i haven't settled on a strong opinion yet, lets see if @devongovett has a strong opinion