Improved Ground truth tags #8586
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve modifications to the 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Outside diff range and nitpick comments (3)
cvat-ui/src/components/annotation-page/tag-annotation-workspace/styles.scss (1)
100-102: LGTM! Consider adding a comment for better maintainability.The new class appropriately uses the design system's grid unit for consistent spacing and follows the project's naming conventions.
Consider adding a brief comment to document the purpose of this class:
+// Adds vertical spacing between frame tags in the canvas annotation view .cvat-canvas-annotation-frame-tags { margin-bottom: $grid-unit-size; }cvat-ui/src/components/annotation-page/tag-annotation-workspace/frame-tags.tsx (2)
Line range hint
19-41: Consider adding explicit return type for tagClassNameThe implementation looks good, but we could improve type safety slightly.
- const tagClassName = useCallback((tag: ObjectState) => { + const tagClassName = useCallback((tag: ObjectState): string => {
Line range hint
61-78: Add missing closable prop for GT tagsThe GT tags are missing the
closableprop which is needed to show the delete button. This might be why GT tags couldn't be deleted as mentioned in the PR objectives.<Tag className={tagClassName(tag)} color={tag.label.color} onClose={() => { onRemoveState(tag); }} key={tag.clientID} + closable >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- cvat-ui/src/components/annotation-page/tag-annotation-workspace/frame-tags.tsx (4 hunks)
- cvat-ui/src/components/annotation-page/tag-annotation-workspace/styles.scss (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
cvat-ui/src/components/annotation-page/tag-annotation-workspace/frame-tags.tsx (4)
8-16: LGTM: Clean migration to React hooksThe transition from Redux connect HOC to hooks (
useDispatch,useSelector) improves code maintainability and follows modern React practices.
Line range hint
44-60: LGTM: Proper handling of regular tagsThe implementation correctly filters and renders non-GT tags with proper removal functionality.
84-84: LGTM: Proper use of React.memoGood optimization to prevent unnecessary re-renders of the component.
45-45: Verify styling improvementsThe PR objectives mention improvements to GT and non-GT tag styling. Let's verify the CSS classes are properly styled.
Also applies to: 62-62
✅ Verification successful
Let me try a different search strategy to find the styling information.
Based on the search results, I can now generate a final response as we have found the styling information in the codebase.
Styling improvements are properly implemented
The styling changes are well-structured and properly implemented in
cvat-ui/src/components/annotation-page/tag-annotation-workspace/styles.scss:
- Base tag styling with
.cvat-frame-tagsclass provides consistent layout with centered content.cvat-frame-tagand.cvat-frame-tag-highlightedclasses handle tag appearance and hover effects.cvat-canvas-annotation-frame-tagsprovides proper spacing with margin- The styling supports both GT and non-GT tags through proper class inheritance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for related CSS classes in styles.scss files rg -l "cvat-canvas-annotation-frame-tags|cvat-canvas-ground-truth-frame-tags|cvat-frame-tag" --type scssLength of output: 135
Script:
#!/bin/bash # Search for related CSS classes in all files, not limiting to scss rg -l "cvat-canvas-annotation-frame-tags|cvat-canvas-ground-truth-frame-tags|cvat-frame-tag" # Also search for any style-related content for these classes rg "\.cvat-canvas-annotation-frame-tags|\.cvat-canvas-ground-truth-frame-tags|\.cvat-frame-tag" -A 5Length of output: 4935
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8586 +/- ##
===========================================
+ Coverage 74.25% 74.28% +0.03%
===========================================
Files 403 403
Lines 43315 43315
Branches 3914 3914
===========================================
+ Hits 32162 32177 +15
+ Misses 11153 11138 -15
|
cvat-ui/src/components/annotation-page/tag-annotation-workspace/frame-tags.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/components/annotation-page/tag-annotation-workspace/frame-tags.tsx
Outdated
Show resolved
Hide resolved
|
Motivation and context
Fixed several issues:
(GT)mark inside of GT job which is not correctxbutton inside the GT jobOther improvements:
frame-tagscomponent code->
Related #8547
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor