feat: scrollable tabs list #38855
feat: scrollable tabs list #38855
Conversation
# Conflicts: # app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.tsx # app/client/packages/design-system/ads/src/DismissibleTab/index.ts
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
IDE Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts (1)
5-12: Consider using CSS variables for dimensions.Hard-coded values should be replaced with CSS variables for better maintainability.
export const Root = styled.div` display: flex; align-items: center; overflow: hidden; white-space: nowrap; position: relative; - height: 32px; + height: var(--ads-v2-spaces-8); `;app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (2)
16-19: Use CSS variables for scroll area dimensions.Magic numbers in SCROLL_AREA_STYLE should be replaced with CSS variables.
const SCROLL_AREA_STYLE = { - height: 34, - top: 1, + height: 'var(--ads-v2-spaces-8)', + top: 'var(--ads-v2-spaces-px)', };
21-81: Consider adding error boundary.The component should be wrapped in an error boundary to gracefully handle rendering failures.
Would you like me to provide an implementation for the error boundary component?
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx (1)
42-60: Consider extracting tab selection logic into a separate utility function.The tab selection logic after closure is complex. Consider extracting it into a separate utility function for better maintainability and testability.
+ const getNextActiveTab = ( + tabs: Array<{ id: string; label: string }>, + closedTabIndex: number, + currentActiveId: string + ) => { + if (closedTabIndex >= tabs.length) { + const nextIndex = Math.max(0, closedTabIndex - 1); + return tabs[nextIndex].id; + } + return tabs[closedTabIndex].id; + }; const handleClose = (tabId: string) => () => { const closedTabIndex = tabs.findIndex((tab) => tab.id === tabId); const filteredTabs = tabs.filter((tab) => tab.id !== tabId); setTabs(filteredTabs); if (activeTabId === tabId && filteredTabs.length) { - if (closedTabIndex >= filteredTabs.length) { - const nextIndex = Math.max(0, closedTabIndex - 1); - const nextTab = filteredTabs[nextIndex]; - setActiveTabId(nextTab.id); - } else { - const nextTab = filteredTabs[closedTabIndex]; - setActiveTabId(nextTab.id); - } + setActiveTabId(getNextActiveTab(filteredTabs, closedTabIndex, activeTabId)); } };app/client/packages/design-system/ads/src/ScrollArea/ScrollArea.tsx (1)
36-44: Improve type safety for ref handling.The ref handling logic could benefit from better type safety and simplification.
useEffect( function init() { - const currentRef = - (typeof ref === "function" ? null : ref?.current) || localRef.current; + const currentRef = ref && 'current' in ref ? ref.current : localRef.current; if (currentRef) initialize(currentRef); }, [initialize, ref], );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.tsx(3 hunks)app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts(1 hunks)app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx(1 hunks)app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.types.ts(1 hunks)app/client/packages/design-system/ads/src/DismissibleTab/index.ts(1 hunks)app/client/packages/design-system/ads/src/ScrollArea/ScrollArea.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/DismissibleTab/index.ts (1)
3-4: LGTM! Clean barrel file exports.The new exports are well-organized and follow the established pattern.
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.types.ts (1)
4-10: Consider adding validation for minimum children.The children prop accepts single or multiple tabs, but doesn't enforce a minimum requirement. Consider adding a runtime check or type constraint to ensure at least one tab is always present.
export interface DismissibleTabBarProps { children: | React.ReactElement<DismissibleTabProps> - | React.ReactElement<DismissibleTabProps>[]; + | [React.ReactElement<DismissibleTabProps>, ...React.ReactElement<DismissibleTabProps>[]]; onTabAdd: () => void; disableAdd?: boolean; }app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTab.tsx (1)
20-27: LGTM! Well-implemented event handling.The handleClose implementation correctly prevents event propagation and is properly memoized.
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx
Outdated
Show resolved
Hide resolved
…ssibleTabBar.styles.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx (2)
42-60: Simplify tab closure logic and add validation.Consider adding validation and simplifying the logic:
const handleClose = (tabId: string) => () => { + if (tabs.length === 0) return; + const closedTabIndex = tabs.findIndex((tab) => tab.id === tabId); const filteredTabs = tabs.filter((tab) => tab.id !== tabId); setTabs(filteredTabs); - if (activeTabId === tabId && filteredTabs.length) { - if (closedTabIndex >= filteredTabs.length) { - const nextIndex = Math.max(0, closedTabIndex - 1); - const nextTab = filteredTabs[nextIndex]; + if (activeTabId !== tabId || !filteredTabs.length) return; - setActiveTabId(nextTab.id); - } else { - const nextTab = filteredTabs[closedTabIndex]; + const nextIndex = closedTabIndex >= filteredTabs.length + ? Math.max(0, closedTabIndex - 1) + : closedTabIndex; - setActiveTabId(nextTab.id); - } - } + setActiveTabId(filteredTabs[nextIndex].id); };
31-33: Add prop validation for containerWidth.Consider adding validation to ensure containerWidth is positive and reasonable.
interface StoryProps extends DismissibleTabBarProps { + /** Width of the container in pixels. Must be between 200 and 600. */ containerWidth: number; } + +const isValidWidth = (width: number) => width >= 200 && width <= 600;app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts (1)
5-12: Extract animation duration to CSS variable.Consider moving the hardcoded animation duration to a CSS variable for consistency and maintainability.
export const animatedLeftBorder = (showLeftBorder: boolean) => css` - transition: border-color 0.5s ease; + transition: border-color var(--ads-v2-transition-duration-base) ease; border-left: 1px solid transparent; border-left-color: ${showLeftBorder ? "var(--ads-v2-color-border-muted)" : "transparent"}; `;app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx (1)
71-83: Add error handling for scrollIntoView.Consider adding try-catch block to handle browsers that don't support smooth scrolling.
useEffect( function scrollAddedTabIntoView() { const element = sentinelRightRef.current; if (element && totalChildren > prevTotalChildren) { + try { element.scrollIntoView({ inline: "center", behavior: "smooth", }); + } catch (error) { + console.warn('Smooth scrolling not supported:', error); + element.scrollIntoView(); + } } }, [totalChildren, prevTotalChildren], );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx(1 hunks)app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts(1 hunks)app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.stories.tsx (1)
66-73: Use functional updates for state changes.Direct mutation of ref value could lead to issues. Consider using functional updates for safer state management.
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx
Outdated
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx
Show resolved
Hide resolved
| useEffect( | ||
| function scrollAddedTabIntoView() { | ||
| const element = sentinelRightRef.current; | ||
|
|
||
| if (element && totalChildren > prevTotalChildren) { | ||
| element.scrollIntoView({ | ||
| inline: "center", | ||
| behavior: "smooth", | ||
| }); | ||
| } | ||
| }, | ||
| [totalChildren, prevTotalChildren], | ||
| ); |
There was a problem hiding this comment.
This is not the same as activeTab. A previously added tab, could get active outside of a tabs selection and that would be needed a scroll into view
There was a problem hiding this comment.
Yes, it's not meant to be active, we only need to scroll to a newly added tab, which will be marked active.
There was a problem hiding this comment.
So the consumer handles the scroll to an active tab that is not new?
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.styles.ts
Show resolved
Hide resolved
app/client/packages/design-system/ads/src/DismissibleTab/DismissibleTabBar.tsx
Show resolved
Hide resolved
Description
Adding a scrollable list template component for DismissibleTab.
Fixes #37692
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13028881854
Commit: 2007e08
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 29 Jan 2025 10:42:35 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
DismissibleTabBarcomponent with advanced tab management capabilities.Improvements
ScrollArea.Design System Updates
DismissibleTabBarto showcase functionality.