[WIKI-319] chore: remove bottom border when toolbar is hidden#6943
[WIKI-319] chore: remove bottom border when toolbar is hidden#6943sriramveeraghanta merged 2 commits intopreviewfrom
Conversation
WalkthroughThe changes introduce a new boolean prop, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageEditorToolbarRoot
participant usePageFilters
participant PageToolbar
User->>PageEditorToolbarRoot: Render editor page
PageEditorToolbarRoot->>usePageFilters: Get isStickyToolbarEnabled
PageEditorToolbarRoot->>PageToolbar: Pass isHidden prop
PageToolbar->>PageToolbar: Apply CSS classes based on isHidden
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
aaryan610
changed the title
|
Pull Request Linked with Plane Work Items Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/core/components/pages/editor/toolbar/mobile-root.tsx(2 hunks)web/core/components/pages/editor/toolbar/root.tsx(1 hunks)web/core/components/pages/editor/toolbar/toolbar.tsx(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/core/components/pages/editor/toolbar/mobile-root.tsx (1)
web/core/hooks/use-page-filters.ts (1)
usePageFilters(21-116)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
web/core/components/pages/editor/toolbar/mobile-root.tsx (1)
8-8: Implementation looks good!Correctly imports and uses the
usePageFiltershook to access theisStickyToolbarEnabledflag, which is then passed to thePageToolbarcomponent as theisHiddenprop. This change aligns with the PR objective of controlling toolbar visibility and its border.Also applies to: 25-25, 33-33
web/core/components/pages/editor/toolbar/toolbar.tsx (2)
18-18: Good implementation of the isHidden propThe addition of the
isHiddenprop to thePropstype and its proper destructuring in the component function body is clean and follows best practices.Also applies to: 67-69
101-108: Well-implemented transition effect for toolbar visibilityThe conditional styling with opacity and pointer-events provides a smooth transition when showing/hiding the toolbar. Using
pointer-events-nonewithopacity-0is a good approach as it ensures hidden elements don't capture clicks while allowing for a smooth transition effect.web/core/components/pages/editor/toolbar/root.tsx (2)
29-29: Good derived state for toolbar visibilityCreating a derived
shouldHideToolbarvalue that combines both conditions (sticky toolbar status and content editability) is a clean approach that makes the code more readable and maintainable.
46-48: Good simplification of conditional renderingThe conditional rendering logic for the
PageToolbarhas been simplified and now correctly passes theisHiddenprop based on the derivedshouldHideToolbarvalue. This is a cleaner approach than having multiple conditions in the JSX.
| "hidden md:flex items-center relative min-h-[52px] page-toolbar-content px-page-x border-b border-transparent transition-all duration-200 ease-in-out", | ||
| { | ||
| "wide-layout": isFullWidth, | ||
| "border-custom-border-200": true, | ||
| } |
There was a problem hiding this comment.
Issue with border styling logic
The border is set to transparent by default but then border-custom-border-200 is applied unconditionally with "border-custom-border-200": true. This means the border will always be visible regardless of whether the toolbar is hidden or not, which contradicts the PR objective of removing the border when the toolbar is hidden.
The border visibility should be conditional on the toolbar visibility state. Apply this fix:
"hidden md:flex items-center relative min-h-[52px] page-toolbar-content px-page-x border-b border-transparent transition-all duration-200 ease-in-out",
{
"wide-layout": isFullWidth,
- "border-custom-border-200": true,
+ "border-custom-border-200": !shouldHideToolbar,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "hidden md:flex items-center relative min-h-[52px] page-toolbar-content px-page-x border-b border-transparent transition-all duration-200 ease-in-out", | |
| { | |
| "wide-layout": isFullWidth, | |
| "border-custom-border-200": true, | |
| } | |
| "hidden md:flex items-center relative min-h-[52px] page-toolbar-content px-page-x border-b border-transparent transition-all duration-200 ease-in-out", | |
| { | |
| "wide-layout": isFullWidth, | |
| "border-custom-border-200": !shouldHideToolbar, | |
| } |
* chore: remove border when toolbar is hidden * chore: add stricter conditions
2 participants
Description
This PR removes the unnecessary bottom border from the editor toolbar when toolbar is hidden
Type of Change
Media
Summary by CodeRabbit
New Features
Style
Bug Fixes