-
Notifications
You must be signed in to change notification settings - Fork 10
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(FloatingPanel): new component #1611
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis update introduces a new FloatingPanel component along with its comprehensive support suite, including documentation, Storybook stories, styles, tests, and helper functions for dynamic styles and animation management. Additionally, a minor update in the AppFrame styling establishes relative positioning for improved layout behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant FP as FloatingPanel Component
participant Anim as useAnimations Hook
participant HS as getFloatingStyles Helper
U->>FP: Toggle visibility (isVisible)
FP->>Anim: Initiate animation with given duration
FP->>HS: Compute styles based on placement and size
HS-->>FP: Return calculated CSS styles
FP->>U: Render FloatingPanel with updated styling and ARIA attributes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (2)
🪧 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
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (18)
packages/react-components/src/components/FloatingPanel/FloatingPanel.module.scss (1)
1-29
: Clean BEM-structured styles with potential improvements.The stylesheet effectively handles placement variations and animations. Consider:
- The hardcoded min-height (100px) might be too restrictive
- Missing max-width/max-height constraints could cause layout issues with large content
- Consider adding transform transitions for smoother animations
.#{$base-class} { position: absolute; - transition: all var(--transition-duration-moderate-2) ease-in-out; + transition: opacity var(--transition-duration-moderate-2) ease-in-out, transform var(--transition-duration-moderate-2) ease-in-out; opacity: 0; z-index: $stacking-context-level-popover; box-shadow: var(--shadow-fixed-bottom); background-color: var(--surface-primary-default); min-height: 100px; + max-height: 80%; + overflow: auto; &--bottom, &--top { right: 0; left: 0; + max-width: 100%; } &--left, &--right { top: 0; bottom: 0; + max-width: 80%; } &--visible { opacity: 1; } }packages/react-components/src/components/FloatingPanel/helpers.ts (2)
3-34
: Consider using switch statement for better readabilityThe function logic is sound, but using separate if statements for each placement makes the code slightly less readable. A switch statement would be more appropriate here.
export const getFloatingStyles = ( placement: IFloatingPanelProps['placement'], isOpen: boolean, panelHeight: number, panelWidth: number ) => { - if (placement === 'bottom') { - return { - top: !isOpen ? '100%' : `calc(100% - ${panelHeight}px)`, - }; - } - - if (placement === 'top') { - return { - bottom: !isOpen ? '100%' : `calc(100% - ${panelHeight}px)`, - }; - } - - if (placement === 'right') { - return { - left: !isOpen ? '100%' : `calc(100% - ${panelWidth}px)`, - }; - } - - if (placement === 'left') { - return { - right: !isOpen ? '100%' : `calc(100% - ${panelWidth}px)`, - }; - } + switch(placement) { + case 'bottom': + return { + top: !isOpen ? '100%' : `calc(100% - ${panelHeight}px)`, + }; + case 'top': + return { + bottom: !isOpen ? '100%' : `calc(100% - ${panelHeight}px)`, + }; + case 'right': + return { + left: !isOpen ? '100%' : `calc(100% - ${panelWidth}px)`, + }; + case 'left': + return { + right: !isOpen ? '100%' : `calc(100% - ${panelWidth}px)`, + }; + default: + return {}; + } - - return {}; }
3-8
: Consider adding return typeAdding TypeScript return type would improve type safety and documentation.
export const getFloatingStyles = ( placement: IFloatingPanelProps['placement'], isOpen: boolean, panelHeight: number, panelWidth: number -) => { +): Record<string, string> => {packages/react-components/src/components/FloatingPanel/FloatingPanel.spec.tsx (2)
10-15
: Consider testing screen readers can access the componentThe tests cover basic visibility, but not accessibility for screen readers.
it('should not be visible by default', () => { const { container } = renderComponent({}); expect(container.firstChild).not.toBeInTheDocument(); + // Also verify it's not announced to screen readers });
26-33
: Test is good but consider adding additional placement testsThe test correctly verifies children rendering, but lacks tests for different placement scenarios.
Consider adding tests for each placement option (top, bottom, left, right) to verify the component's positioning behavior works correctly.
packages/react-components/src/components/FloatingPanel/FloatingPanel.mdx (1)
18-26
: Add more examples for different placementsThe documentation clearly explains the positioning requirement but would benefit from examples for each placement option.
Consider adding examples for top, bottom, left, and right placements to demonstrate the full capabilities of the component.
packages/react-components/src/components/FloatingPanel/stories-helpers.tsx (6)
10-16
: Fix typo in props interfaceThe interface contains a typo in property names.
interface IExampleAppContentProps { - isBottomBarVisisble: boolean; - isTopBarVisisble: boolean; - isLeftBarVisisble: boolean; - isRightBarVisisble: boolean; + isBottomBarVisible: boolean; + isTopBarVisible: boolean; + isLeftBarVisible: boolean; + isRightBarVisible: boolean; handleBarVisibilityChange: (kind: string) => void; }
18-24
: Fix property names in component declarationProperty names contain typos and should be fixed for consistency.
export const ExampleAppContent: FC<IExampleAppContentProps> = ({ - isBottomBarVisisble, - isTopBarVisisble, - isLeftBarVisisble, - isRightBarVisisble, + isBottomBarVisible, + isTopBarVisible, + isLeftBarVisible, + isRightBarVisible, handleBarVisibilityChange, }) => {
42-44
: Update property names in component usageUpdate the variable names to match the corrected property names.
<Switch size="compact" - on={isBottomBarVisisble} + on={isBottomBarVisible} onChange={() => handleBarVisibilityChange('bottom')} />
50-52
: Update property names in component usageUpdate the variable names to match the corrected property names.
<Switch size="compact" - on={isTopBarVisisble} + on={isTopBarVisible} onChange={() => handleBarVisibilityChange('top')} />
58-60
: Update property names in component usageUpdate the variable names to match the corrected property names.
<Switch size="compact" - on={isLeftBarVisisble} + on={isLeftBarVisible} onChange={() => handleBarVisibilityChange('left')} />
66-68
: Update property names in component usageUpdate the variable names to match the corrected property names.
<Switch size="compact" - on={isRightBarVisisble} + on={isRightBarVisible} onChange={() => handleBarVisibilityChange('right')} />packages/react-components/src/components/FloatingPanel/FloatingPanel.tsx (3)
21-28
: Consider using ResizeObserver for responsive dimensionsThe component tracks panel dimensions only on mount, but might not update if content changes dynamically.
- const { isOpen, isMounted } = useAnimations({ - isVisible: isVisible, - elementRef: floatingElementWrapperRef, - animationDuration: 500, - }); + const { isOpen, isMounted } = useAnimations({ + isVisible, + elementRef: floatingElementWrapperRef, + animationDuration: 500, + });Consider implementing a ResizeObserver to dynamically update dimensions when content changes:
useEffect(() => { if (!isMounted) return; const updateDimensions = () => { if (floatingElementWrapperRef?.current) { setPanelHeight(floatingElementWrapperRef.current.getBoundingClientRect().height || 0); setPanelWidth(floatingElementWrapperRef.current.getBoundingClientRect().width || 0); } }; updateDimensions(); const resizeObserver = new ResizeObserver(updateDimensions); if (floatingElementWrapperRef.current) { resizeObserver.observe(floatingElementWrapperRef.current); } return () => { resizeObserver.disconnect(); }; }, [isMounted]);
38-47
: Potential performance improvement for dimension calculationsThe component recalculates dimensions whenever
isMounted
changes, but it could be more efficient.- useEffect(() => { - if (isMounted) { - setPanelHeight( - floatingElementWrapperRef?.current?.getBoundingClientRect().height || 0 - ); - setPanelWidth( - floatingElementWrapperRef?.current?.getBoundingClientRect().width || 0 - ); - } - }, [isMounted]); + useEffect(() => { + if (isMounted && floatingElementWrapperRef.current) { + const { height, width } = floatingElementWrapperRef.current.getBoundingClientRect(); + setPanelHeight(height || 0); + setPanelWidth(width || 0); + } + }, [isMounted]);
53-64
: Consider using more semantic ARIA attributesThe component uses a custom role, but standard ARIA roles might be more appropriate.
- <div - role="floating-panel" - aria-visible={isOpen} + <div + role="dialog" + aria-hidden={!isOpen}The standard "dialog" role would be more semantic than a custom "floating-panel" role, and "aria-hidden" is more standard than "aria-visible".
packages/react-components/src/components/FloatingPanel/FloatingPanel.stories.tsx (3)
98-101
: Fix typo in variable namesVariable names contain a typo that should be corrected for consistency.
- const [isBottomBarVisisble, setIsBottomBarVisisble] = useState(true); - const [isTopBarVisisble, setIsTopBarVisisble] = useState(false); - const [isLeftBarVisisble, setIsLeftBarVisisble] = useState(false); - const [isRightBarVisisble, setIsRightBarVisisble] = useState(false); + const [isBottomBarVisible, setIsBottomBarVisible] = useState(true); + const [isTopBarVisible, setIsTopBarVisible] = useState(false); + const [isLeftBarVisible, setIsLeftBarVisible] = useState(false); + const [isRightBarVisible, setIsRightBarVisible] = useState(false);Remember to update all references to these variables throughout the file.
103-131
: Consider simplifying panel visibility managementThe current implementation closes all panels before opening a new one, but this could be made more maintainable.
- const closePanels = () => { - setIsBottomBarVisisble(false); - setIsTopBarVisisble(false); - setIsLeftBarVisisble(false); - setIsRightBarVisisble(false); - }; - - const handleBarVisibilityChange = (kind: string) => { - switch (kind) { - case 'bottom': - closePanels(); - setIsBottomBarVisisble(!isBottomBarVisisble); - break; - case 'top': - closePanels(); - setIsTopBarVisisble(!isTopBarVisisble); - break; - case 'left': - closePanels(); - setIsLeftBarVisisble(!isLeftBarVisisble); - break; - case 'right': - closePanels(); - setIsRightBarVisisble(!isRightBarVisisble); - break; - default: - break; - } - }; + const handleBarVisibilityChange = (kind: string) => { + const stateUpdates = { + bottom: false, + top: false, + left: false, + right: false, + }; + + if (kind in stateUpdates) { + const currentState = + kind === 'bottom' ? isBottomBarVisible : + kind === 'top' ? isTopBarVisible : + kind === 'left' ? isLeftBarVisible : + isRightBarVisible; + + stateUpdates[kind as keyof typeof stateUpdates] = !currentState; + + setIsBottomBarVisible(stateUpdates.bottom); + setIsTopBarVisible(stateUpdates.top); + setIsLeftBarVisible(stateUpdates.left); + setIsRightBarVisible(stateUpdates.right); + } + };This approach is more scalable and less prone to errors as it uses a declarative pattern.
293-318
: Consider consolidating panel examplesThe examples for different panel placements contain duplicated structure.
Consider creating a reusable panel content component to reduce duplication:
type PanelContentProps = { placement: 'top' | 'bottom' | 'left' | 'right'; }; const PanelContent = ({ placement }: PanelContentProps) => { const isHorizontal = placement === 'top' || placement === 'bottom'; return isHorizontal ? ( <div className="horizontal-panel"> <Button kind="primary" onClick={() => {}}>Save</Button> <Button onClick={() => {}}>Cancel</Button> </div> ) : ( <div className="vertical-panel"> {Array.from({ length: 5 }).map((_, i) => ( <Checkbox key={i}>Option {i + 1}</Checkbox> ))} </div> ); };Then use it in your panels:
<FloatingPanel isVisible={isBottomBarVisible} placement="bottom"> <PanelContent placement="bottom" /> </FloatingPanel>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/react-components/src/components/AppFrame/AppFrame.module.scss
(1 hunks)packages/react-components/src/components/FloatingPanel/FloatingPanel.mdx
(1 hunks)packages/react-components/src/components/FloatingPanel/FloatingPanel.module.scss
(1 hunks)packages/react-components/src/components/FloatingPanel/FloatingPanel.spec.tsx
(1 hunks)packages/react-components/src/components/FloatingPanel/FloatingPanel.stories.tsx
(1 hunks)packages/react-components/src/components/FloatingPanel/FloatingPanel.tsx
(1 hunks)packages/react-components/src/components/FloatingPanel/FloatingPanelStories.css
(1 hunks)packages/react-components/src/components/FloatingPanel/helpers.ts
(1 hunks)packages/react-components/src/components/FloatingPanel/stories-helpers.tsx
(1 hunks)packages/react-components/src/components/FloatingPanel/types.ts
(1 hunks)packages/react-components/src/hooks/useAnimations.ts
(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/react-components/src/components/FloatingPanel/FloatingPanelStories.css
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: chromatic-deployment
🔇 Additional comments (11)
packages/react-components/src/components/AppFrame/AppFrame.module.scss (1)
67-67
: Excellent positioning addition for FloatingPanel support.Setting
position: relative
creates the proper positioning context for absolute elements like the new FloatingPanel component.packages/react-components/src/components/FloatingPanel/types.ts (1)
1-12
: Well-structured interface with clear documentation.The interface follows TypeScript best practices with meaningful property names and clear JSDoc comments.
packages/react-components/src/hooks/useAnimations.ts (3)
9-9
: Good extensibility with animation duration parameter.Adding configurable animation duration improves the hook's flexibility.
22-22
: Sensible default animation duration.300ms is a good default that balances perceived speed with smooth transitions.
47-50
: Fallback timeout now properly uses the configured duration.This ensures consistency between CSS transitions and the JavaScript fallback.
packages/react-components/src/components/FloatingPanel/FloatingPanel.mdx (1)
28-30
: The Component API section looks goodClear and well-structured documentation of the component's API.
packages/react-components/src/components/FloatingPanel/FloatingPanel.tsx (2)
1-11
: Clean imports with good organizationImports are well-structured with proper grouping of React, third-party, and internal dependencies.
14-20
: Well-defined props with sensible defaultsThe component props are clearly defined with appropriate default values for
isVisible
andplacement
.packages/react-components/src/components/FloatingPanel/FloatingPanel.stories.tsx (3)
33-40
: Well-configured Storybook metadataThe Storybook configuration includes appropriate parameters for chromatic testing and fullscreen layout.
74-91
: Simple and effective default storyThe Default story demonstrates basic functionality clearly with toggle behavior.
285-292
: Good usage example of bottom-positioned panelThe bottom-positioned FloatingPanel is well-implemented with clear action buttons.
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.
👍
Resolves: #1205
Description
New component
Storybook
https://feature-1205--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit