Fix filtered modal tagging #4723
Conversation
WalkthroughThe changes encompass modifications across multiple components, enhancing functionality, simplifying interfaces, and improving the structure of types and props. Notable updates include the addition of new methods for interaction in the modal sidebar, the integration of keyboard events for enhanced user experience, and the refinement of type definitions to improve maintainability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModalSidebar
participant SampleWrapper
User->>SampleWrapper: Press "c" key
SampleWrapper->>ModalSidebar: Toggle hovering state
ModalSidebar-->>User: Update UI
Warning Review ran into problemsProblems (1)
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 Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts-snapshots/labels-chromium-darwin.pngis excluded by!**/*.png,!**/*.pnge2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts-snapshots/labels-chromium-linux.pngis excluded by!**/*.png,!**/*.png
Files selected for processing (6)
- app/packages/components/src/components/PillButton/PillButton.tsx (1 hunks)
- app/packages/core/src/components/Modal/Sample.tsx (2 hunks)
- app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3 hunks)
- e2e-pw/src/oss/poms/modal/modal-sidebar.ts (2 hunks)
- e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (7 hunks)
- fiftyone/server/routes/tag.py (1 hunks)
Files skipped from review due to trivial changes (1)
- app/packages/components/src/components/PillButton/PillButton.tsx
Additional context used
Path-based instructions (4)
app/packages/core/src/components/Modal/Sample.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.e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Sidebar/Entries/GroupEntries.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.
Additional comments not posted (17)
app/packages/core/src/components/Modal/Sample.tsx (2)
8-8: ImportuseKeyDownis correctly added.The
useKeyDownhook is imported appropriately for use in the component.
29-29:useKeyDownusage is correct.The
useKeyDownhook is effectively used to toggle thehoveringstate with the "c" key, enhancing interactivity.fiftyone/server/routes/tag.py (1)
72-72: Verify the impact of removing filters.Replacing the
filtersparameter with an empty dictionary simplifies the function call. Ensure this aligns with the intended behavior and does not affect functionality.Run the following script to check for any usage of the
filtersparameter in the codebase that might be affected by this change:e2e-pw/src/oss/poms/modal/modal-sidebar.ts (5)
16-21:applyFiltermethod implementation is correct.The method correctly interacts with the checkbox element to apply a filter.
23-27:applySearchmethod implementation is correct.The method correctly performs the search operation by filling the input and pressing "Enter".
29-31:clearGroupFiltersmethod implementation is correct.The method correctly interacts with the button to clear filters for a specific group.
33-36:clickFieldDropdownmethod implementation is correct.The method correctly interacts with the dropdown element.
74-76:toggleLabelCheckboxmethod implementation is correct.The method correctly toggles the checkbox for the specified field.
e2e-pw/src/oss/specs/smoke-tests/tagger.spec.ts (6)
4-4: Import ofModalPomis appropriate.The import of
ModalPomis necessary for the new modal interactions in the tests.
12-14: Test setup changes are appropriate.The inclusion of
modaland removal ofgridActionsRowalign with the new testing requirements for modal interactions.Also applies to: 19-20
Line range hint
42-54: Test case changes for tag aggregation are appropriate.The test case correctly interacts with the grid and tagger components to verify tag aggregation numbers.
Line range hint
57-75: Test case for adding a new sample tag is well-structured.The test case effectively validates the addition of a new sample tag to all samples using the grid and tagger components.
Line range hint
78-98: Test case for adding a new label tag is well-structured.The test case effectively ensures the correct addition of a new label tag to all samples.
102-128: Test case for adding a label tag in modal is comprehensive.The test case thoroughly tests the modal interactions and label tagging functionality.
app/packages/core/src/components/Sidebar/Entries/GroupEntries.tsx (3)
35-36: Addition ofdataCytoPillEntryis beneficial.The optional
dataCyproperty enhances testing capabilities by allowing data attributes for testing.
47-52: PassingdataCytoPillButtonis appropriate.The change ensures that
dataCyis utilized for testing purposes in the rendered component.
311-332: Inclusion ofdataCyattributes inPathGroupEntryis beneficial.The use of
dataCyattributes improves the traceability and testing of UI components.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- fiftyone/server/routes/tag.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- fiftyone/server/routes/tag.py
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/core/src/components/Modal/Sample.tsx (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/components/Modal/Sample.tsx
| arrow?: boolean; | ||
| highlight?: boolean; | ||
| text?: string; | ||
| icon?: JSX.Element; | ||
| arrow?: boolean; | ||
| id?: string; | ||
| onClick: (event: Event) => void; | ||
| open?: boolean; | ||
| style?: React.CSSProperties; | ||
| text?: string; |
There was a problem hiding this comment.
🔤 🚀
(although my personal pref is to first list sorted non-optional fields, followed by optional fields)
There was a problem hiding this comment.
Set up an automated lint rule and I will abide 😄
What changes are proposed in this pull request?
When tagging in the modal, the updated samples returned by the
/tagroute incorrectly include sidebar filters which results in seemingly lost data when tagging with sidebar filters applied.As an aside, I added
cas a hot key to hide the actions row because I could not figure out how to trigger themouseleaveevent. This allows for a pure screenshot of the looker canvasHow is this patch tested? If it is not, please explain why.
Added e2e spec
Release Notes
Fixed tagging in the modal when sidebar filters are applied
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Bug Fixes
Documentation