-
Notifications
You must be signed in to change notification settings - Fork 63
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
LG-4251: remove portal and use descendants pattern in tabs #2348
Conversation
|
d7a64f8
to
8bd7a41
Compare
Size Change: +111 B (+0.01%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
dcd5013
to
2d63657
Compare
435225d
to
f24a640
Compare
packages/tabs/src/Tabs/Tabs.tsx
Outdated
tabTitleElements[controlledSelected].focus(); | ||
}, [controlledSelected, isControlled, tabTitleElements]); |
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.
This will technically fire if/when tabTitleElements
or isControlled
changes.
I can imagine a case where some external event causes tabs to be added/removed, which would trigger this effect
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.
If I comment out this useEffect
locally, changing selected
in Storybook does not update the focus (only the green "selected" indicator)
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.
@TheSonOfThomp to avoid having the effect run based off external events when tabs are added/removed, I opted to leave out the useEffect
. When testing on the Controlled
story instance, focus seems to update as intended. Let me know if you think otherwise, and I can add it back in
f24a640
to
d397627
Compare
70208b7
to
eb5e49c
Compare
selected={selectedTab} | ||
setSelected={setSelectedTab} |
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.
598f711
to
9aeec87
Compare
c0b6b76
to
ea8c648
Compare
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 would restructure the TabPanel
component to remove the clone
call, but what you have here works! Left a few nits, but overall LGTM
@@ -0,0 +1,12 @@ | |||
export function getActiveAndEnabledIndices( |
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.
imo splitting this into separate getActive...
and getEnabled...
would be a cleaner DX
If performance is a concern, we can memoize the getEnabled...
function so we're not filtering/mapping twice, an leverage it within getActive...
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.
can address in a follow-up
(e.key === keyMap.ArrowRight | ||
? activeIndex + 1 | ||
: activeIndex - 1 + numberOfEnabledTabs) % numberOfEnabledTabs |
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.
this is probably just a prettier thing, but this was hard for me to parse. I read this as only the ArrowLeft case would perform the "mod count" operation
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.
can address and make more explicit in a follow-up
* Add @leafygreen-ui/descendants dependency * Use descendants pattern in tabs package * Remove Portal dependency from tabs package * Reorg descendant contexts * Rename tabs event handlers * Rewrite effect * Refactor * Controlled story and consolidate controlled tabs tests * Clean up Controlled story
* Add and export class names (#2335) * LG-3270: fixed tab title height (#2351) * LG-4251: remove portal and use descendants pattern in tabs (#2348) * Add @leafygreen-ui/descendants dependency * Use descendants pattern in tabs package * Remove Portal dependency from tabs package * Reorg descendant contexts * Rename tabs event handlers * Rewrite effect * Refactor * Controlled story and consolidate controlled tabs tests * Clean up Controlled story * LG-2728: migrate from Box to Polymorphic in tabs package (#2359) * LG-2728: migrate from Box to Polymorphic * Address feedback * LG-4147: forceRenderAllTabPanels prop with test util updates (#2357) * Add expect statement to test spec * Refactor TabTitle props and TabPanel * Refactor handleKeyDown and util * Changeset * Fix type * Address feedback
✍️ Proposed changes
@leafygreen-ui/descendants
infra@leafygreen-ui/portal
dependencyWill make final changeset after all tabs changes are merged to integration branch
🎟 Jira ticket: LG-4251
✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changesFor new components
🧪 How to test changes