[WEB-3203] fix: stickies height, overflow #6484
Conversation
WalkthroughThe pull request introduces refinements to several components related to sticky notes and their editor functionality. The changes primarily focus on styling adjustments, layout modifications, and minor improvements to component rendering across multiple files. Key modifications include updating class names, adjusting height and padding settings, and introducing conditional rendering logic for better responsiveness across different page contexts. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web/core/components/stickies/sticky/root.tsx (1)
77-77: Consider adding a max-height constraint for better UX.While adding
overflow-y-scrollhelps with content overflow, removing the max-height constraint entirely might lead to excessively tall sticky notes. Consider adding a responsive max-height constraint using CSS variables or viewport units.- className={cn("w-full h-fit flex flex-col rounded group/sticky overflow-y-scroll", className)} + className={cn("w-full h-fit max-h-[80vh] flex flex-col rounded group/sticky overflow-y-scroll", className)}web/core/components/editor/sticky-editor/editor.tsx (1)
84-87: Improved toolbar visibility transitions.The toolbar animation and visibility logic has been simplified while maintaining smooth transitions:
- Fixed height of 60px ensures consistency
- Scale and opacity transitions provide smooth animations
- Proper use of
invisibleclass prevents interaction when hiddenConsider adding a comment explaining the transition values for future maintainability.
className={cn( "transition-all duration-300 ease-out origin-top px-4 h-[60px]", + // Transition values: + // - duration-300: 300ms transition duration + // - ease-out: smooth deceleration + // - origin-top: scale from top edge { "max-h-[60px] opacity-100 scale-y-100": isFocused, "max-h-0 opacity-0 scale-y-0 invisible": !isFocused, } )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/core/components/editor/sticky-editor/editor.tsx(3 hunks)web/core/components/editor/sticky-editor/toolbar.tsx(1 hunks)web/core/components/stickies/layout/sticky-dnd-wrapper.tsx(1 hunks)web/core/components/stickies/sticky/inputs.tsx(3 hunks)web/core/components/stickies/sticky/root.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- web/core/components/stickies/layout/sticky-dnd-wrapper.tsx
- web/core/components/editor/sticky-editor/toolbar.tsx
🔇 Additional comments (7)
web/core/components/stickies/sticky/root.tsx (1)
Line range hint
77-81: Verify scroll behavior on mobile devices.The
overflow-y-scrollimplementation might need additional touch handling for smooth scrolling on mobile devices. Consider testing the scroll behavior across different devices and screen sizes.✅ Verification successful
Native scroll behavior is sufficient for mobile devices
The current implementation using
overflow-y-scrollrelies on the browser's native touch handling, which is well-supported across modern mobile devices and provides a smooth scrolling experience out of the box. No additional touch handling is necessary.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any touch event handlers or mobile-specific scroll implementations rg -g '*.{ts,tsx}' 'touch|scroll|mobile' web/core/components/stickiesLength of output: 609
web/core/components/stickies/sticky/inputs.tsx (4)
2-2: LGTM! Clean import additions.The new imports are properly organized and necessary for the implemented changes.
Also applies to: 9-9
29-30: LGTM! Good use of Next.js navigation hook.The
usePathnamehook is appropriately used to determine the current context.Also applies to: 35-35
81-86: Verify the max-height values for different contexts.The conditional max-height implementation looks good, but let's ensure these specific values work well across all scenarios:
- Default: max-h-[540px]
- Stickies page: max-h-[588px]
Also, the scrollbar implementation with
overflow-y-scrollis a good addition for handling content overflow.Could you verify if these height values are sufficient for all use cases? For example:
- Different screen sizes
- Various content lengths
- Mobile responsiveness
89-89: LGTM! Clean border removal.Removing border and padding from the parent container helps maintain consistent styling.
web/core/components/editor/sticky-editor/editor.tsx (2)
6-7: LGTM! Better import organization.Good categorization of the TSticky type import under "plane types".
62-62: LGTM! Consistent border styling.The border and rounded corners provide a clean, consistent look while maintaining the sticky note aesthetic.
Description
This PR adds a proper max height to each sticky note along with fixing other overflow issues.
Type of Change
Summary by CodeRabbit
Style Updates
Improvements
Performance