Responsive saved view dropdown #6638
Conversation
WalkthroughThe changes modify the Selection and Option components to improve label text display and dropdown width synchronization. A new Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Areas for attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/packages/components/src/components/Selection/Selection.tsx (1)
72-107: Width sync works on open but won’t react to live container resizesUsing
containerRef.current?.clientWidthinPaperProps.sx.widthcorrectly aligns the dropdown width with the container when it opens. However, if the container width changes while the menu is open (e.g., window resize in a responsive layout), the menu width will not update until the component re-renders/reopens because the width is not stored in state or tied to a resize observer. If truly responsive-in-flight behavior is required, consider measuring into state or using aResizeObserver; otherwise this is fine as a pragmatic trade‑off.Also applies to: 114-118, 120-126
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
app/packages/components/src/components/Selection/Option.tsx(2 hunks)app/packages/components/src/components/Selection/Selection.tsx(3 hunks)app/packages/components/src/components/Selection/styledComponents.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (app/CODING_STANDARDS.md)
app/**/*.{jsx,tsx}: Use useState or useReducer for UI-only local state that resets with component
Use Context API (useContext) for small bounded trees with static-ish data, keeping contexts minimal to avoid re-renders
Files:
app/packages/components/src/components/Selection/Selection.tsxapp/packages/components/src/components/Selection/Option.tsx
app/**/*.{jsx,tsx,ts}
📄 CodeRabbit inference engine (app/CODING_STANDARDS.md)
app/**/*.{jsx,tsx,ts}: Use Jotai (preferred) or Recoil (legacy) for reactive global state
Only export domain hooks that read/mutate atoms; never export raw useAtomValue, useSetAtomValue, useRecoilValue, or useSetRecoilValue in components
Files:
app/packages/components/src/components/Selection/Selection.tsxapp/packages/components/src/components/Selection/Option.tsxapp/packages/components/src/components/Selection/styledComponents.ts
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/components/src/components/Selection/Selection.tsxapp/packages/components/src/components/Selection/Option.tsxapp/packages/components/src/components/Selection/styledComponents.ts
🧠 Learnings (1)
📚 Learning: 2024-10-22T16:25:08.017Z
Learnt from: minhtuev
Repo: voxel51/fiftyone PR: 4957
File: app/packages/components/src/components/Toast/Toast.tsx:29-36
Timestamp: 2024-10-22T16:25:08.017Z
Learning: When reviewing React components in `app/packages/components/src/components/*`, verify component imports carefully before suggesting missing imports, especially when components are imported via destructuring from packages like `mui/material`.
Applied to files:
app/packages/components/src/components/Selection/Selection.tsx
🧬 Code graph analysis (1)
app/packages/components/src/components/Selection/Option.tsx (1)
app/packages/components/src/components/Selection/styledComponents.ts (4)
SelectionRow(68-80)RowLabelContainer(62-66)TextContainer(12-17)LabelText(19-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: modified-files
🔇 Additional comments (1)
app/packages/components/src/components/Selection/styledComponents.ts (1)
12-25: LabelText + flex-based TextContainer look appropriate for wrapping/truncation controlThe flex TextContainer plus
LabelTextwithwrap?: booleancleanly separate layout from text-wrapping behavior and preserve the previous truncation-by-default semantics while enabling wrapped labels where needed. No further changes required here.
| <SelectionRow ref={hoverRef} onClick={onClick} data-cy={dataCy}> | ||
| <RowLabelContainer> | ||
| <TextContainer> | ||
| {preDecorator} {label} | ||
| {preDecorator} | ||
| <LabelText wrap>{label}</LabelText> | ||
| </TextContainer> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the component file around the specified lines
wc -l app/packages/components/src/components/Selection/Option.tsxRepository: voxel51/fiftyone
Length of output: 123
🏁 Script executed:
# Read the component to understand the layout structure
sed -n '40,70p' app/packages/components/src/components/Selection/Option.tsxRepository: voxel51/fiftyone
Length of output: 973
🏁 Script executed:
# Find styled component definitions in this file
rg -n "const (LabelText|TextContainer|EditBox|SelectionRow|RowLabelContainer)" app/packages/components/src/components/Selection/Option.tsx -A 3Repository: voxel51/fiftyone
Length of output: 42
🏁 Script executed:
# Check imports at top of Option.tsx
head -30 app/packages/components/src/components/Selection/Option.tsxRepository: voxel51/fiftyone
Length of output: 759
🏁 Script executed:
# Look for styled components file in Selection directory
fd -e ts -e tsx app/packages/components/src/components/Selection/ | grep -i "style\|styled"Repository: voxel51/fiftyone
Length of output: 561
🏁 Script executed:
# Check if styledComponents are defined in the same file or imported
grep -n "LabelText\|TextContainer\|EditBox" app/packages/components/src/components/Selection/Option.tsx | head -20Repository: voxel51/fiftyone
Length of output: 277
🏁 Script executed:
# Find and read the styledComponents file
find app/packages/components/src/components/Selection -name "*styled*"Repository: voxel51/fiftyone
Length of output: 128
🏁 Script executed:
# List all files in Selection directory
ls -la app/packages/components/src/components/Selection/Repository: voxel51/fiftyone
Length of output: 560
🏁 Script executed:
# Read the styledComponents file to check CSS properties
cat -n app/packages/components/src/components/Selection/styledComponents.tsRepository: voxel51/fiftyone
Length of output: 4238
Add horizontal padding or width constraint to prevent wrapped text from rendering behind the EditBox icons
The TextContainer has width: 100% with no reserved horizontal space, while LabelText uses flex: 1 and allows wrapping. The EditBox is absolutely positioned at right: 0.5rem, which means multi-line labels can extend beneath it. Although the EditBox only appears on hover/select, wrapped text will still be partially obscured when the icons are visible. Add padding-right to TextContainer or a max-width constraint to LabelText to reserve horizontal space for the icons.
🤖 Prompt for AI Agents
In app/packages/components/src/components/Selection/Option.tsx around lines
50-55, the TextContainer and LabelText allow wrapped label text to extend
beneath the absolutely positioned EditBox at right:0.5rem; reserve horizontal
space by adding either padding-right to TextContainer (matching the EditBox
width + margin) or by applying a max-width/right padding to LabelText (or
reducing its flex) so multi-line labels never underlap the icons; update styles
accordingly and verify wrapped labels remain fully visible when the EditBox
appears.
lanzhenw
left a comment
lanzhenw
left a comment
There was a problem hiding this comment.
changes look good to me.
| PaperProps: { | ||
| sx: { | ||
| width: containerRef.current?.clientWidth, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Nice fix. now dropdown width now matches container width instead of fixed 270px
What changes are proposed in this pull request?
https://voxel51.atlassian.net/browse/FOEPD-2700
Make saved view dropdown responsive and show full name
How is this patch tested? If it is not, please explain why.
Screen.Recording.2025-12-03.at.4.16.24.PM.mov
Release Notes
Make saved view dropdown responsive and show entire name
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.