Skip to content
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

Fixed styles on Consensus management page #9178

Merged
merged 8 commits into from
Mar 12, 2025

Conversation

klakhov
Copy link
Contributor

@klakhov klakhov commented Mar 6, 2025

Motivation and context

The PR fixes broken styles on consensus management page:
image

Also I extracted -control styles to base.scss so the header and inner styles can be re-used on different pages: consensus, quality for tasks and projects, analytics

And added notification error for quality/consensus settings (previously no notification was shown)
image

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have created a changelog fragment
  • [ ] I have updated the documentation accordingly
  • [ ] I have added tests to cover my changes
  • [ ] I have linked related issues (see GitHub docs)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@klakhov klakhov requested a review from bsekachev as a code owner March 6, 2025 08:12
@klakhov klakhov added the ui/ux label Mar 6, 2025
@klakhov klakhov requested a review from nmanovic as a code owner March 6, 2025 09:07
@codecov-commenter
Copy link

codecov-commenter commented Mar 6, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.34%. Comparing base (78b48cb) to head (832e5f0).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #9178   +/-   ##
========================================
  Coverage    73.34%   73.34%           
========================================
  Files          449      449           
  Lines        45863    45868    +5     
  Branches      3915     3917    +2     
========================================
+ Hits         33637    33641    +4     
- Misses       12226    12227    +1     
Components Coverage Δ
cvat-ui 77.11% <80.00%> (+<0.01%) ⬆️
cvat-server 70.32% <ø> (ø)
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 98 to 109
.cvat-control-inner {
min-height: inherit;
display: flex;
flex-direction: column;
background: white;
border-radius: $grid-unit-size;

@media screen and (min-width: $scroll-breakpoint) {
height: 100%;
min-height: auto;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Not really sure about names cvat-control-*. The name is not associated to pages where it is used in my mind. What do you think about page-header, page-tabs, page-inner? Maybe it is getting to abstract and need to think some more..

  2. Try to use mixins. It looks they do exactly what you want: https://sass-lang.com/documentation/at-rules/mixin/

Copy link
Contributor Author

@klakhov klakhov Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about cvat-management-page-... type of naming? page.. feels really too abstract, it can be any page. Like we curretly have quality management, analytics management, consensus management.

@klakhov klakhov mentioned this pull request Mar 11, 2025
6 tasks
@klakhov
Copy link
Contributor Author

klakhov commented Mar 12, 2025

@bsekachev applied

@bsekachev bsekachev merged commit 2b835e2 into develop Mar 12, 2025
34 checks passed
archibald1418 pushed a commit that referenced this pull request Mar 12, 2025
archibald1418 added a commit that referenced this pull request Mar 14, 2025
#### Depends on #9178

### Motivation and context
Test coverage for merging feature in consensus jobs (#8953)
Commits were cherry-picked from #9190

### How has this been tested?

### Case 1: check new merge buttons exist and are visible.
New freshly created jobs have status 'new'. Trying to merge new jobs
should result in error notifications and a HTTP 400 status code. All
notifications are closed before the next testcase

### Case 2: Check consensus management page
Consensus management page has settings for consensus quorum and minimum
annotation overlap. Check that the page has fields for both options. If
a field is filled incorrectly the form turns red and a message appears
under the form. Trying to save options with erroneous forms should do
nothing - no requests are sent, no errors shown.

### Case 3: Create annotations and check that job replicas merge
correctly
Create annotations with one rectangle in job replicas. The rectangle is
moved with a step in each job's first frame. This means that after
merging, the consensus job will have a rectangle which has to fully
match a rectangle inside the replica in the middle.

- Create annotations.
- Merge consensus jobs. No errors should appear. A notification message
validates the merge, close it
- Go inside consensus job, remember the rectangle's positions
- Go back to the task page using browser's 'Back' button
- After loading the task page, consensus job's status should read
**competed**
- Open the middle job replica. Rectangle should be identical to the
previous one

### Checklist

- [ ] I submit my changes into the `develop` branch
- [ ] I have created a changelog fragment
- [ ] I have updated the documentation accordingly
- [ ] I have added tests to cover my changes
- [ ] I have linked related issues (see [GitHub docs](

https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword))

### License

- [ ] I submit _my code changes_ under the same [MIT License](
https://github.com/cvat-ai/cvat/blob/develop/LICENSE) that covers the
project.
  Feel free to contact the maintainers if that's a concern.

---------

Co-authored-by: Oleg Valiulin <[email protected]>
Co-authored-by: Kirill Lakhov <[email protected]>
@cvat-bot cvat-bot bot mentioned this pull request Mar 24, 2025
@bsekachev bsekachev deleted the kl/fix-consensus-management branch March 27, 2025 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants