-
Notifications
You must be signed in to change notification settings - Fork 3.2k
feat: BROS-596: Remove the top bar on the labeling stream #8822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
feat: BROS-596: Remove the top bar on the labeling stream #8822
Conversation
… Labeling Stream. Adapts style of left-side buttons on BottomBar.
✅ Deploy Preview for label-studio-docs-new-theme canceled.
|
✅ Deploy Preview for heartex-docs canceled.
|
✅ Deploy Preview for label-studio-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for label-studio-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8822 +/- ##
===========================================
- Coverage 65.77% 58.04% -7.74%
===========================================
Files 811 552 -259
Lines 63432 40017 -23415
Branches 10690 10675 -15
===========================================
- Hits 41723 23228 -18495
+ Misses 21706 16786 -4920
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/git merge
|
|
This looks fantastic 🔥 @ricardoantoniocm. One question, with the changes here, what happens with the annotation tabs? Do they remain the same as before but take up the whole row of spacing now? |
|
Thanks @bmartel ! The annotation tabs take up the whole row of spacing now on Quick View and the Review Stream. Minus the View All / Add Annotation buttons. |
|
|
||
| {!isViewAll && ( | ||
| <div className={cn("bottombar").elem("section").toClassName()}> | ||
| <div className={cn("model-actions").toClassName()}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this classname going to fail on testing selectors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probable. But I think the best approach would be to add data-testids to the buttons. I'll add those.
| look="string" | ||
| disabled={!historyEnabled || !store.canGoPrevTask} | ||
| onClick={store.prevTask} | ||
| style={{ background: !isFF(FF_DEV_3873) && "none", backgroundColor: isFF(FF_DEV_3873) && "none" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this FF_DEV_3873 gating, also why the inline style?
| <div | ||
| className={cn("current-task") | ||
| .elem("history-controls") | ||
| .mod({ newui: isFF(FF_DEV_3873) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yyassi-heartex Do you recall what this FF_DEV_3873 is for? I feel there are a lot of changes in this PR that are hinging on its lingering existence, which makes it hard to understand what is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmartel we are basically on UI v3 right now. We had v1 which was finally removed some time ago (FF_1170). And we still have v2 hidden behind feature flag. It's enabled for everyone and is ready to be removed, but it's everywhere, including some tests, so we have to fix them first. One day, one day...
| const [initialCommentLength, setInitialCommentLength] = useState(0); | ||
| const [visibleComments, setVisibleComments] = useState(0); | ||
|
|
||
| useEffect(() => { | ||
| store.commentStore.setAddedCommentThisSession(false); | ||
|
|
||
| const reactionDisposer = reaction( | ||
| () => store.commentStore.comments.map((item) => item.isDeleted), | ||
| (result) => { | ||
| setVisibleComments(result.filter((item) => !item).length); | ||
| }, | ||
| ); | ||
|
|
||
| return () => { | ||
| reactionDisposer?.(); | ||
| }; | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (store.commentStore.addedCommentThisSession) { | ||
| setInitialCommentLength(visibleComments); | ||
| } | ||
| }, [store.commentStore.addedCommentThisSession]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically that's just a TopBar/CurrentTask.jsx moved to BottomBar. This file was not used at all, so it can be fully rewritten with no doubts. And apparently these files overlapped enough to have this specific diffs instead of full rewrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of the code serves two purposes:
- Tracks visible (non-deleted) comments: The first useEffect sets up a MobX reaction that monitors the comment store. Whenever a comment's isDeleted status changes, it recalculates how many comments are still visible and updates the visibleComments state.
- Captures baseline comment count: The second useEffect watches for when a user adds their first comment in the current session (addedCommentThisSession flag). When this happens, it saves the current visible comment count as the initialCommentLength. This baseline is likely used to determine how many new comments were added during this session (by comparing current count to initial count).
This component needs to distinguish between comments that existed when the task was opened versus comments added during the current annotation session. This is used to help determine whether a task can be postponed. See lines 52-54.
When FF_DEV_4174 is enabled, the postpone functionality requires at least one "visible" comment.
| if (store.hasInterface("annotations:comments") && isFF(FF_DEV_4174)) { | ||
| canPostpone = canPostpone && store.commentStore.addedCommentThisSession && visibleComments >= initialCommentLength; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure of this change, or why its necessary to move the topbar CurrentTask down to the bottombar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the context @bmartel https://humansignal.slack.com/archives/C016L55AYJX/p1762284677894759
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we changed the logic of canPostpone here. Maybe I am not seeing the ask in the messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just about the moving the CurrentTask to the bottom. The canPostpone shouldn't have changed. I'll check that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here — it's just how it was in original TopBar file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FF_DEV_4174 is specifically turned ON for just one of our customers:
| <div className={cn("current-task").elem("task-id").toClassName()}> | ||
| <div | ||
| className={cn("current-task").elem("task-id").toClassName()} | ||
| style={{ fontSize: isFF(FF_DEV_3873) ? 12 : 14 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question here about the need for a FF based inline style
|
/git merge
|
This pull request refactors and improves the BottomBar component in the editor UI, focusing on layout, style consistency, and user interaction. It moves the
CurrentTaskcomponent to the Bottom Bar, enhances button usability and accessibility, and standardizes spacing and visual separation between controls. The changes also update styles to use theme variables and improve feature flag handling for conditional UI elements.Recordings
Before (LSO)
https://www.loom.com/share/3a6f0e71075143568147b8b1fb55ef92
After (LSO)
https://www.loom.com/share/9626f00290bc4ce58da6dcbdbddbf272
Component and Layout Refactoring:
CurrentTaskcomponent to the BottomBar, which displays the current task ID, history controls, and task counter, with improved feature flag support and conditional rendering. [1] [2] [3] [4]Actionscomponent to use semanticdivcontainers instead ofSpace, grouping action buttons and model actions with clearer separation and updated markup. [1] [2] [3]UI and Style Improvements:
Button and Interaction Enhancements:
Utility and Import Updates:
BemWithSpecificContext) for more flexible class name generation.Icon and Color Consistency: