-
Notifications
You must be signed in to change notification settings - Fork 61.2k
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
fix: Fix RTL direction spacing errors and sidebar dragging functional… #6274
base: main
Are you sure you want to change the base?
Conversation
@YaoweiZou is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes involve updates to the sidebar's styling and functionality across multiple components. In the SCSS file, margin rules are replaced with gap properties for better spacing, and new classes are introduced to enhance layout flexibility. In the TypeScript files, imports and functions are modified to streamline sidebar dragging behavior, including adjustments for language directionality and the removal of redundant props. Overall, the modifications simplify the rendering logic and improve the user interface. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as Sidebar Component
participant L as getLang()
U->>S: Initiate sidebar drag
S->>L: Request current language
L-->>S: Return language code (e.g., "ar")
alt Language is Arabic
S->>S: Reverse drag direction (negate d)
else Other language
S->>S: Apply normal drag behavior
end
S->>U: Update sidebar position accordingly
Poem
✨ Finishing Touches
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 (
|
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 (6)
app/components/sidebar.tsx (5)
15-15
: Check broader RTL languages
Currently, the drag direction is inverted only ifgetLang()
equals"ar"
. Consider covering all RTL locales (e.g., Hebrew, Persian) if needed.
107-109
: Applying dynamic sidebar width
Using a CSS variable to store and update the width is neat. Consider testing orientation changes and re-renders for consistency.Would you like a quick verification script to confirm CSS changes under simulated orientation events?
156-164
: Async MCP check
ThecheckMcpStatus
logs an error only in the console. Consider user-facing error handling or graceful fallback if MCP initialization fails.
285-289
: Remove stale commented code
The code segment at lines 285-289 is fully commented out. Delete it if it's no longer relevant to keep the code base lean.
292-301
: Refined sidebar usage
Replacing the old logic with<SideBarContainer>
and removing thenarrow
references reduces complexity. If the commented tail code is obsolete, remove it fully.app/components/chat-list.tsx (1)
63-69
: Unified layout
Eliminating thenarrow
-based conditional rendering simplifies the UI. Confirm it still looks acceptable on smaller displays.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/icons/panel-left-close.svg
is excluded by!**/*.svg
📒 Files selected for processing (5)
app/components/chat-list.tsx
(3 hunks)app/components/home.module.scss
(6 hunks)app/components/home.tsx
(2 hunks)app/components/sidebar.tsx
(7 hunks)app/constant.ts
(5 hunks)
🔇 Additional comments (20)
app/components/sidebar.tsx (9)
8-8
: SVG icon import
The additional icon import looks correct. Verify that its dimensions and styling blend well with the rest of the UI.
29-29
: Import for selector
AddingSelector
aligns well with the new discovery functionality.
80-84
: RTL drag logic
Negatingd
for Arabic is an effective approach. Verify behavior when switching between LTR and RTL at runtime (if supported).
87-87
: Sidebar width floor check
Restricting the width strictly to> MIN_SIDEBAR_WIDTH
may prevent narrower layouts. Confirm it matches intended design specs.
111-111
: Refined return value
Only returningonDragStart
removes complexity. Ensure no other components depend on the removedshouldNarrow
.
118-127
: Smooth event binding
DestructuringonDragStart
and applying the className viaclsx
is clean and consistent with your style approach.
144-154
: Discovery and MCP states
The new hooks and state variables (showDiscoverySelector
,mcpEnabled
) significantly expand sidebar capabilities. Check that re-renders remain performant.
168-179
: Header close button
The new top-level close button is introduced but appears to have no click handler. If it's not intended for future use, consider removing it to avoid confusion.
203-251
: Enhanced sidebar bar
Including Discovery, Mask, and MCP features in the bar is a nice addition for discoverability. Good job centralizing these actions.app/components/chat-list.tsx (2)
87-87
: Dropping the narrow prop
Removing thenarrow
prop fromChatList
aligns with the new sidebar approach. This cleanup should reduce complexity.
139-139
: Always confirm chat deletion
Asking for confirmation every time is user-friendly. Ensure the confirmation text is translated or localized as needed.app/components/home.tsx (2)
185-187
: Inline route checks
Returning<AuthPage />
or<Sd />
directly is simpler. Verify that mergingisSd
andisSdNew
flows does not disrupt user expectations for separate pages.
196-212
: Always show sidebar
Rendering the<SideBar>
for all routes (except early returns) unifies layout logic. Test on smaller screens to confirm it doesn't hamper usability.app/constant.ts (1)
102-102
: LGTM! Increased minimum sidebar width.The change from 230px to 260px provides more space for the sidebar content, which should help resolve the RTL direction spacing issues mentioned in the PR objectives.
app/components/home.module.scss (6)
55-55
: LGTM! Improved spacing in sidebar header bar.Using
gap
property instead of margins is a better approach for consistent spacing between flex items.
94-102
: LGTM! Well-structured header actions layout.The new
.header-actions
class with flex layout and nested.second-actions
provides a clean and maintainable way to organize header buttons.
188-188
: LGTM! Simplified chat item styling.Replacing complex styling (border-radius, margin-bottom, box-shadow) with a simple bottom border creates a cleaner, more consistent look.
198-198
: LGTM! Improved selected chat item visibility.Using background color instead of border color for selected items provides better visual feedback.
254-254
: LGTM! Consistent spacing in sidebar actions.Using
gap
property for spacing between sidebar actions aligns with the modern CSS best practices.
269-272
: LGTM! Fixed RTL sidebar drag functionality.The RTL-specific adjustments to the sidebar drag handle's position correctly handle right-to-left layouts, addressing the issue mentioned in the PR objectives.
@Leizhenpeng 这个 PR 还需要更多的信息才能合并吗?如果是,还需要什么? |
@Leizhenpeng Does this PR need more information to merge? If so, what else is needed? |
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
修复 RTL 布局下 sidebar 拖动异常。[Bug] RTL character causes sidebar dragging issue. #3975
修复 RTL 布局下 sidebar 部分按钮间距异常。
Summary by CodeRabbit