regression: SidepanelItem visual issues
#36798
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a regression in the sidepanel list where incorrect spacing/gap was being applied. The changes address layout issues by restructuring how the SidepanelListWrapper component handles refs and removing unnecessary padding from the SidePanel container.
- Restructures SidepanelListWrapper to properly handle ref forwarding
- Removes bottom padding from SidePanel container
- Updates class name reference for keyboard shortcuts
- Fixes typos in localization strings
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/i18n/src/locales/en.i18n.json | Corrects "hear" to "here" in multiple description strings |
| apps/meteor/client/views/navigation/sidepanel/SidepanelListWrapper.tsx | Restructures component to wrap content in a div and handle refs differently |
| apps/meteor/client/views/navigation/sidepanel/SidePanel.tsx | Removes bottom padding from container Box |
| apps/meteor/client/views/navigation/sidebar/hooks/useShortcutOpenMenu.ts | Updates class name from 'rcx-sidebar-item' to 'rcx-sidebar-v2-item' |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import { useTranslation } from 'react-i18next'; | ||
|
|
||
| import { useSidebarListNavigation } from '../../../sidebar/RoomList/useSidebarListNavigation'; | ||
| import { useSidebarListNavigation } from '../sidebar/RoomList/useSidebarListNavigation'; |
There was a problem hiding this comment.
The import path has been changed but appears incorrect. The path '../sidebar/RoomList/useSidebarListNavigation' suggests going up one level to 'sidebar', but from the sidepanel directory, the correct path should be '../../../sidebar/RoomList/useSidebarListNavigation' as it was originally.
|
|
||
| return <SidepanelList aria-label={t('Channels')} ref={mergedRefs} {...props} />; | ||
| return ( | ||
| <SidepanelList aria-label={t('Channels')} ref={sidebarListRef}> | ||
| <div ref={ref} {...props} /> | ||
| </SidepanelList> |
There was a problem hiding this comment.
The component structure has been fundamentally changed from forwarding refs to the SidepanelList to wrapping content in a div. This breaks the expected API where the forwarded ref should reference the SidepanelList component itself, not an inner div wrapper.
dougfabris
changed the title
SidepanelItem visual issues
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-7.10.0 #36798 +/- ##
==================================================
+ Coverage 65.97% 66.02% +0.04%
==================================================
Files 3285 3287 +2
Lines 110045 110192 +147
Branches 20837 20855 +18
==================================================
+ Hits 72607 72750 +143
- Misses 34765 34766 +1
- Partials 2673 2676 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
apps/meteor/client/views/navigation/sidepanel/SidepanelListWrapper.tsx
Outdated
Show resolved
Hide resolved
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
2 participants
Proposed changes (including videos or screenshots)
Important
This change is under feature preview
Introduced here: #36049
Depends on: RocketChat/fuselage#1742
Issue(s)
Steps to test or reproduce
Further comments
SIDE2-119
SIDE2-127
SIDE2-153
SIDE2-154