-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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(js): inbox tabs #6149
feat(js): inbox tabs #6149
Conversation
export const InboxTab = (props: { label: string; class?: string }) => { | ||
const style = useStyle(); | ||
|
||
// TODO: Replace with actual count from API |
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 fetching logic for the counts will be in the separate PR/ticket
<span class={style('tabsTabLabel', 'nt-text-sm nt-font-medium')}>{props.label}</span> | ||
<span | ||
class={style('tabsTabCount', 'nt-rounded-full nt-bg-primary nt-px-[6px] nt-text-primary-foreground nt-text-sm')} | ||
> | ||
{count >= 100 ? '99+' : count} | ||
</span> |
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.
not sure if these should be part of the Tabs.Tab
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.
Generally, they should, but in our case, we'd only do it for the appearance keys. When we add an element, we are forced to add an appearance key anyway, and more specific that the primitive one. This means that we would redeclare one anyway when using Tab.Label
for example. I'm fine with them not being actual components.
tabs: Array<{ label: string; value: Array<string> }>; | ||
}; | ||
|
||
export const InboxTabs = (props: InboxTabsProps) => { |
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 stateful component for tabs
onMount(() => { | ||
const tabsListEl = tabsList(); | ||
if (!tabsListEl) return; | ||
|
||
const tabs = [...tabsListEl.querySelectorAll('[role="tab"]')]; | ||
|
||
const observer = new IntersectionObserver( | ||
(entries) => { | ||
let visibleTabIds = entries | ||
.filter((entry) => entry.isIntersecting && entry.intersectionRatio === 1) | ||
.map((entry) => entry.target.id); | ||
|
||
if (tabs.length === visibleTabIds.length) { | ||
setVisibleTabs(props.tabs.filter((tab) => visibleTabIds.includes(tab.label))); | ||
observer.disconnect(); | ||
return; | ||
} | ||
|
||
visibleTabIds = visibleTabIds.slice(0, -1); | ||
setVisibleTabs(props.tabs.filter((tab) => visibleTabIds.includes(tab.label))); | ||
setDropdownTabs(props.tabs.filter((tab) => !visibleTabIds.includes(tab.label))); | ||
observer.disconnect(); | ||
}, | ||
{ root: tabsListEl } | ||
); | ||
|
||
for (const tabElement of tabs) { | ||
observer.observe(tabElement); | ||
} | ||
}); |
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.
Calculate which tabs are fully visible in the parent container. The ones that are not visible will be shown in the dropdown.
To have enough space to render the dropdown we remove one tab from the visible tabs list in line 35;
<Tabs.List ref={setTabsList}> | ||
{props.tabs.map((tab) => ( | ||
<InboxTab label={tab.label} class="nt-invisible" /> | ||
))} | ||
</Tabs.List> |
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.
These tabs are rendered hidden at the beginning (otherwise there would be a blink). Then we calculate which ones are fully visible and don't overflow in the parent container. Later we render the visible tabs.
packages/js/src/ui/components/primitives/Popover/PopoverContent.tsx
Outdated
Show resolved
Hide resolved
'moreActions__dropdownItemLabelContainer', | ||
'moreActions__dropdownItemLeftIcon', | ||
//Actions dropdown | ||
'actionsContainer', |
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.
check the comment in MoreTabsDropdown
createEffect(() => { | ||
const handleTabKey = (event: KeyboardEvent) => { | ||
if (event.key !== 'Tab') { | ||
return; | ||
} | ||
|
||
const tabs = tabsContainer()?.querySelectorAll('[role="tab"]'); | ||
if (!tabs || !document.activeElement) { | ||
return; | ||
} | ||
|
||
setKeyboardNavigation(Array.from(tabs).includes(document.activeElement)); | ||
}; | ||
|
||
document.addEventListener('keyup', handleTabKey); | ||
|
||
return onCleanup(() => document.removeEventListener('keyup', handleTabKey)); | ||
}); |
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.
listen to the tab
keyboard events and check if document.activeElement
is the "tab dom element", if yes allow keyboard navigation with ArrowLeft
or ArrowRight
createEffect(() => { | ||
const handleArrowKeys = (event: KeyboardEvent) => { | ||
if (!keyboardNavigation() || (event.key !== 'ArrowLeft' && event.key !== 'ArrowRight')) { | ||
return; | ||
} | ||
|
||
const tabElements = Array.from<HTMLButtonElement>(tabsContainer()?.querySelectorAll('[role="tab"]') ?? []); | ||
const tabIds = tabElements.map((tab) => tab.id); | ||
let currentIndex = tabIds.indexOf(activeTab()); | ||
const length = tabIds.length; | ||
let activeIndex = currentIndex; | ||
let newTab = activeTab(); | ||
if (event.key === 'ArrowLeft') { | ||
activeIndex = currentIndex === 0 ? length - 1 : currentIndex - 1; | ||
newTab = tabIds[activeIndex]; | ||
} else if (event.key === 'ArrowRight') { | ||
activeIndex = currentIndex === length - 1 ? 0 : currentIndex + 1; | ||
newTab = tabIds[activeIndex]; | ||
} | ||
|
||
tabElements[activeIndex].focus(); | ||
setActiveTab(newTab); | ||
}; | ||
|
||
document.addEventListener('keydown', handleArrowKeys); | ||
|
||
return onCleanup(() => document.removeEventListener('keydown', handleArrowKeys)); | ||
}); |
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 keyboard arrow keys navigation between the tabs; calculations for the next tab to focus and set active tab;
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.
Generally, I'd prefer it if we implemented the tabs more the <Popover>
, as we could set the default tabs styles in a function (like popoverContentVariants()
), and default to those, unless another class
prop is specified. See PopoverContent.tsx
for reference.
We only need one instance of Tabs for now, so I'd say this could also be a cleanup ticket post-release.
d4fa90a
to
d646347
Compare
d646347
to
00000aa
Compare
@@ -51,11 +60,8 @@ export const Inbox = (props: InboxProps) => { | |||
</Button> | |||
)} | |||
/> | |||
<Popover.Content | |||
appearanceKey="inbox__popoverContent" | |||
class={cn(popoverContentVariants(), 'nt-max-w-96 nt-w-full')} |
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.
moved to the variants function
7c8d105
to
ee854a6
Compare
What changed? Why was the change needed?
The Inbox tabs UI component implementation:
NovuUI
Known limitations:
it doesn't support keyboard navigationScreenshots
Screen.Recording.2024-07-24.at.17.08.16.mov
Screen.Recording.2024-07-24.at.19.07.46.mov