Add sidebar value text expansion #4487
Conversation
WalkthroughThe changes in Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 2
Outside diff range and nitpick comments (2)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (2)
Line range hint
74-78: Switch cases informatfunction are falling through.case FRAME_SUPPORT_FIELD: value = `[${value[0]}, ${value[1]}]`; + break; case DATE_FIELD: // @ts-ignore value = formatDate(value.datetime as number); + break; case DATE_TIME_FIELD: // @ts-ignore value = formatDateTime(value.datetime as number, timeZone);Add
breakstatements to prevent unintended fallthrough in switch cases.
Line range hint
293-293: Unnecessary use of React Fragments.- <> + // Remove the fragment if it's not serving any purpose.Consider removing unnecessary React Fragments to clean up the code.
Also applies to: 319-319
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (12 hunks)
Additional context used
Path-based instructions (1)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Biome
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx
[error] 74-75: This case is falling through to the next case. (lint/suspicious/noFallthroughSwitchClause)
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.
[error] 76-78: This case is falling through to the next case. (lint/suspicious/noFallthroughSwitchClause)
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.
[error] 133-133: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 211-215: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 218-222: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 416-421: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 293-293: Avoid using unnecessary Fragment. (lint/complexity/noUselessFragments)
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
[error] 319-319: Avoid using unnecessary Fragment. (lint/complexity/noUselessFragments)
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
Additional comments not posted (5)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (5)
6-6: Added importFRAME_SUPPORT_FIELDfrom@fiftyone/utilities.This import is necessary for the new feature implementation.
16-19: Updated Recoil imports to includeatomFamily,selectorFamily, anduseRecoilValueLoadable.These imports are essential for state management in the updated component.
27-34: IntroducedexpandedPathValueEntryatomFamily for managing the expanded state of entries.This is a good use of Recoil's atomFamily for state management specific to each path value entry.
37-40: Added a new styled componentTitleDiv.This addition supports the new UI changes for the title display in the sidebar.
55-57: UpdatedScalarDivto handle expanded state.This change allows the text to wrap when the entry is expanded, improving readability.
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx
Outdated
Show resolved
Hide resolved
|
@benjaminpkane couple UX thoughts here:
|
Text selection is still possible. I will add the |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (2)
Line range hint
77-81: Fix switch case fall-through informatfunction.The cases for
FRAME_SUPPORT_FIELD,DATE_FIELD, andDATE_TIME_FIELDare falling through, which could lead to unexpected behavior. Each case should return immediately after setting the value.case FRAME_SUPPORT_FIELD: value = `[${value[0]}, ${value[1]}]`; + return prettify(value as string); case DATE_FIELD: // @ts-ignore value = formatDate(value.datetime as number); + return prettify(value as string); case DATE_TIME_FIELD: // @ts-ignore value = formatDateTime(value.datetime as number, timeZone); + return prettify(value as string);
Line range hint
277-277: Remove unnecessary React Fragments.The use of React Fragments in these cases is redundant and can be safely removed to clean up the code.
- <> + // Directly return the JSX elements without a fragment wrapperAlso applies to: 303-303
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (12 hunks)
Additional context used
Path-based instructions (1)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Biome
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx
[error] 77-78: This case is falling through to the next case. (lint/suspicious/noFallthroughSwitchClause)
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.
[error] 79-81: This case is falling through to the next case. (lint/suspicious/noFallthroughSwitchClause)
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.
[error] 136-136: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 207-207: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 400-405: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
[error] 277-277: Avoid using unnecessary Fragment. (lint/complexity/noUselessFragments)
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
[error] 303-303: Avoid using unnecessary Fragment. (lint/complexity/noUselessFragments)
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
Additional comments not posted (2)
app/packages/core/src/components/Sidebar/Entries/PathValueEntry.tsx (2)
27-34: Introduce browser storage effect for theexpandedPathValueEntryatom.This implementation using
fos.getBrowserStorageEffectForKeyensures that the expansion state of entries persists across browser sessions, enhancing user experience by remembering their preferences.
58-60: Ensure expanded text does not break layout.Adjusting the
white-spaceproperty when the text is expanded is a smart way to handle long strings without affecting the layout negatively. This change enhances readability while maintaining a clean UI.
What changes are proposed in this pull request?
Adds text expansion option via container click.
Screen.Recording.2024-06-12.at.10.46.21.AM.mov
How is this patch tested? If it is not, please explain why.
Locally
Release Notes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Style