-
Notifications
You must be signed in to change notification settings - Fork 3k
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
CVAT UI: batch of fixes #1705
CVAT UI: batch of fixes #1705
Conversation
settings moved from page to modal
Pull Request Test Coverage Report for Build 6136
💛 - Coveralls |
cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/object-item.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/components/header/settings-modal/player-settings.tsx
Outdated
Show resolved
Hide resolved
cvat-ui/src/components/annotation-page/standard-workspace/objects-side-bar/objects-side-bar.tsx
Outdated
Show resolved
Hide resolved
if (prevProps.showAllInterpolationTracks !== showAllInterpolationTracks) { | ||
onFetchAnnotation(); | ||
} | ||
|
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.
Can we dispatch this action immediately after changing settings?
Here: src/containers/header/settings-modal/workspace-settings.tsx
Line 72
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.
@bsekachev, what about example future case when we change value by loading it from server? I think it can be more common way.
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 am not sure I understand you. What value? How are we going to get it from the server?
My idea is that now it is not obvious how the action is called after changing showAllInterpolationTracks
.
@@ -42,7 +42,7 @@ export default function WorkspaceSettingsComponent(props: Props): JSX.Element { | |||
onSwitchAutomaticBordering, | |||
} = props; | |||
|
|||
const minAutoSaveInterval = 5; | |||
const minAutoSaveInterval = 1; |
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 it necessary to change or for test only?
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.
@bsekachev, what is preventing us to decrease this parameter? I guess we discussed it sometime.
cvat-ui/src/components/header/settings-modal/player-settings.tsx
Outdated
Show resolved
Hide resolved
Here is an overview of what got changed by this pull request: Issues
======
- Added 2
See the complete overview on Codacy |
} | ||
|
||
.cvat-settings-modal .ant-modal-body{ |
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.
Codacy found an issue: Expected single space before "{" (block-opening-brace-space-before)
|
||
.cvat-settings-modal .ant-modal-body{ | ||
padding-top: 0; | ||
} |
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.
Codacy found an issue: Unexpected missing end-of-source newline (no-missing-end-of-source-newline)
Motivation and context
This PR resolve several issues and fix some of eslint problems.
Resolve #918
Resolve #1458
Resolve #1514
Resolve #1643
Resolve #1646
Resolve #1666
How has this been tested?
Manually
Checklist
develop
branch- [ ] I have updated the documentation accordingly- [ ] I have added tests to cover my changescvat-core, cvat-data and cvat-ui)
License
Feel free to contact the maintainers if that's a concern.