[GSoC2024] Implemented drag and drop feature #7624
Conversation
KrishavRajSingh
changed the title
There was a problem hiding this comment.
Generally im not sure if this solution is good in UX terms. Design with arrows is a bit outdated and not so comfortable to use (if I want to place 10th attribute to the first place I need to make 10 clicks 😟).
Lets return to original proposal, as the author of 7370 mentiones, it sould be implemented as drag-n-drop. I belive we need to use dots icon that is shown only on hover.
It should be something similar to:
In CVAT, we use react-grid-layout to help us implement draggable and responsible layouts on analytics page and canvas-layout. I would suggest to implement this feature using specific rows-only draggable layout.
Also, please, add a link to the issue you're trying to resolve in PR description. And dont forget to add a changelog entry.
klakhov
requested review from
klakhov
and removed request for
bsekachev and
azhavoro
|
Ok, I will make the changes |
Hey I think react-grid-layout doesn't works on Forms. |
|
What do you mean by doesnt work on Forms? Could you describe the soultion you are trying to implement so we can discuss it? |
I was trying to implement the same but after wrapping with < ReactGridLayout >, this.renderAttributes was not returning anything. Most probably because fieldInstances was empty when calling from < ReactGridLayout >. |
|
I think we dont need to pass |
Hey, I have almost completed it but I am facing a problem. The thing is when we change the layout, I update the attributes list and this layout updates based on this new attributes list. These were the changes that I have made: url. |
|
Hi @KrishavRajSingh , As I can see you are trying to update form after layout changes and then it re-renders. I think we can fix this problem by managing layout somewhere outside of the form. There should be some state for the layout. By default it should be empty, then on first render of the form we set it up. If user makes some changes, we set the state of the layout object. When the form is submitted ( |
|
Also, I would recommend to look at sorting component. It may be found on projects/tasks/cloud storages pages
And this solution will not work when attributes were already saved on the server. |
|
Hi @KrishavRajSingh, |
|
I am looking into this. |
Thanks for this, I was finally able to implement this drag&drop. Drag.Drop_CVAT.mp4 |
KrishavRajSingh
changed the title
KrishavRajSingh
changed the title
WalkthroughThe update introduces a drag-and-drop feature to reorder attributes in the CVAT UI's labels editor. This is achieved by integrating Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant LabelForm
participant ReactGridLayout
User->>LabelForm: Initiates drag-and-drop
LabelForm->>ReactGridLayout: Updates layout
ReactGridLayout->>LabelForm: Confirms new order
LabelForm->>User: Displays updated order
Assessment against linked issues
Tip New Features and ImprovementsReview SettingsIntroduced new personality profiles for code reviews. Users can now select between "Chill" and "Assertive" review tones to tailor feedback styles according to their preferences. The "Assertive" profile posts more comments and nitpicks the code more aggressively, while the "Chill" profile is more relaxed and posts fewer comments. AST-based InstructionsCodeRabbit offers customizing reviews based on the Abstract Syntax Tree (AST) pattern matching. Read more about AST-based instructions in the documentation. Community-driven AST-based RulesWe are kicking off a community-driven initiative to create and share AST-based rules. Users can now contribute their AST-based rules to detect security vulnerabilities, code smells, and anti-patterns. Please see the ast-grep-essentials repository for more information. New Static Analysis ToolsWe are continually expanding our support for static analysis tools. We have added support for Tone SettingsUsers can now customize CodeRabbit to review code in the style of their favorite characters or personalities. Here are some of our favorite examples:
Revamped Settings PageWe have redesigned the settings page for a more intuitive layout, enabling users to find and adjust settings quickly. This change was long overdue; it not only improves the user experience but also allows our development team to add more settings in the future with ease. Going forward, the changes to Miscellaneous
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 CodeRabbit Configration 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: 1
Out of diff range and nitpick comments (2)
cvat/apps/engine/views.py (2)
Line range hint
1967-1967: Convert the f-string to a regular string as it doesn't contain any placeholders.- return Response("{} is an invalid directory".format(directory_param), + return Response("{} is an invalid directory".format(directory_param),
Line range hint
2714-2714: Convert the f-string to a regular string as it doesn't contain any placeholders.- return Response(data='No such file were uploaded', + return Response(data='No such file were uploaded',
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- changelog.d/20240401_121841_krishavrajsingh_drag_drop.md (1 hunks)
- cvat-ui/src/components/labels-editor/label-form.tsx (7 hunks)
- cvat-ui/src/components/labels-editor/styles.scss (1 hunks)
- cvat/apps/engine/migrations/0079_attributespec_updated_time.py (1 hunks)
- cvat/apps/engine/models.py (1 hunks)
- cvat/apps/engine/views.py (2 hunks)
Files skipped from review due to trivial changes (3)
- changelog.d/20240401_121841_krishavrajsingh_drag_drop.md
- cvat-ui/src/components/labels-editor/styles.scss
- cvat/apps/engine/migrations/0079_attributespec_updated_time.py
Additional Context Used
Ruff (3)
cvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2714-2714: f-string without any placeholders
Additional comments not posted (6)
cvat-ui/src/components/labels-editor/label-form.tsx (4)
Line range hint
20-31: Correct integration ofreact-grid-layoutwithWidthProvider.The integration of
react-grid-layoutwithWidthProviderand the creation of theReactGridLayoutconstant are correctly implemented.
70-78: Ensure the layout state is correctly updated and attributes are reordered.The logic for updating the layout state and reordering attributes seems correct. However, ensure that the
updatedLayoutandvalues.attributesare always in sync to avoid potential issues.
464-500: Correct rendering of attributes with drag-and-drop handle.The rendering logic for attributes with the drag-and-drop handle using
react-grid-layoutis correctly implemented.
703-729: Correct integration ofForm.Listwithreact-grid-layout.The integration of
Form.Listwithreact-grid-layoutfor reordering attributes using drag-and-drop is correctly implemented.cvat/apps/engine/models.py (1)
861-862: Correct addition ofupdated_timefield inAttributeSpecmodel.The
updated_timefield withauto_now=Trueis correctly added to theAttributeSpecmodel.cvat/apps/engine/views.py (1)
52-52: The import statement looks good and is necessary for the changes related toAttributeSpec.
The attributes will come from the front-end as it is, then a new variable display_order is introduced on the server side whose value is equal to the index of the attribute in attributes. "display_order" is set for each attribute and then it is saved to the databse. |
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
cvat/apps/engine/views.py (1)
Line range hint
2714-2714: Remove unnecessary f-string.- f"" + ""
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- cvat-ui/src/components/labels-editor/label-form.tsx (7 hunks)
- cvat/apps/engine/migrations/0080_alter_attributespec_options_and_more.py (1 hunks)
- cvat/apps/engine/migrations/0081_alter_attributespec_options.py (1 hunks)
- cvat/apps/engine/models.py (1 hunks)
- cvat/apps/engine/serializers.py (4 hunks)
- cvat/apps/engine/views.py (2 hunks)
Files skipped from review due to trivial changes (2)
- cvat/apps/engine/migrations/0080_alter_attributespec_options_and_more.py
- cvat/apps/engine/migrations/0081_alter_attributespec_options.py
Additional Context Used
Ruff (5)
cvat/apps/engine/serializers.py (2)
711-711: f-string without any placeholders
1146-1146: f-string without any placeholders
cvat/apps/engine/views.py (3)
44-44:
cvat.apps.dataset_manager.viewsimported but unused
1967-1967: f-string without any placeholders
2714-2714: f-string without any placeholders
Additional comments not posted (13)
cvat-ui/src/components/labels-editor/label-form.tsx (4)
8-10: The new imports forRGLandWidthProviderfromreact-grid-layoutare necessary for the drag-and-drop functionality. Ensure thatreact-grid-layoutis listed as a dependency in yourpackage.json.Also applies to: 20-20
31-31: TheReactGridLayoutconstant is correctly defined usingWidthProviderandRGL. This setup is essential for the drag-and-drop functionality.
464-496: The inclusion of the drag-and-drop handle in therenderAttributemethod is correct and aligns with the new feature requirements.
703-730: The integration ofReactGridLayoutwithForm.Listfor the drag-and-drop functionality is correct. Ensure that the layout state is managed correctly and consistently.Verification successful
The integration of
ReactGridLayoutwithForm.Listfor the drag-and-drop functionality is correct. The layout state is being managed correctly usingthis.formRef.current?.setFieldsValueandthis.formRef.current?.getFieldValue.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the layout state management in the `LabelForm` component. # Test: Search for the layout state management. Expect: Proper handling of layout state. rg --type typescript 'layout' cvat-ui/src/components/labels-editor/label-form.tsxLength of output: 116
Script:
#!/bin/bash # Description: Verify the layout state management in the `LabelForm` component. # Test: Search for the layout state management. Expect: Proper handling of layout state. rg --type tsx 'layout' cvat-ui/src/components/labels-editor/label-form.tsxLength of output: 102
Script:
#!/bin/bash # Description: Verify the layout state management in the `LabelForm` component. # Test: Search for the layout state management. Expect: Proper handling of layout state. rg 'layout' cvat-ui/src/components/labels-editor/label-form.tsxLength of output: 482
cvat/apps/engine/models.py (3)
861-861: The addition of theupdated_timefield to theAttributeSpecmodel is a good practice for tracking the last modification time.
862-862: The addition of thedisplay_orderfield to theAttributeSpecmodel will help manage the order of attributes explicitly.
867-867: The addition of theorderingoption in theMetaclass of theAttributeSpecmodel ensures that attributes are ordered by thedisplay_orderfield by default.cvat/apps/engine/serializers.py (6)
238-238: Addition ofdisplay_orderfield is consistent with the new drag-and-drop feature.
242-242: Inclusion ofdisplay_orderin thefieldslist ensures proper serialization.
409-409: Initialization ofdisplay_orderto 0 is necessary for setting the initial order of attributes.
432-432: Setting and incrementingdisplay_orderensures unique ordering of attributes.
438-438: Incrementingdisplay_orderafter saving each attribute ensures correct ordering.
491-492: Addingdisplay_orderto the attributes dictionary ensures correct ordering for new attributes.
| @@ -49,7 +49,7 @@ | |||
| from cvat.apps.engine.frame_provider import FrameProvider | |||
| from cvat.apps.engine.media_extractors import get_mime | |||
| from cvat.apps.engine.models import ( | |||
| ClientFile, Job, JobType, Label, SegmentType, Task, Project, Issue, Data, | |||
| ClientFile, Job, JobType, Label, AttributeSpec, SegmentType, Task, Project, Issue, Data, | |||
There was a problem hiding this comment.
Remove unused import.
- import cvat.apps.dataset_manager.views # pylint: disable=unused-importCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| ClientFile, Job, JobType, Label, AttributeSpec, SegmentType, Task, Project, Issue, Data, | |
| ClientFile, Job, JobType, Label, AttributeSpec, SegmentType, Task, Project, Issue, Data, |
There was a problem hiding this comment.
@klakhov, I have verified the imports in the cvat/apps/engine/views.py file. The import statement import cvat.apps.dataset_manager.views # pylint: disable=unused-import is indeed unused and can be safely removed.
Here is the updated import section without the unused import:
from cvat.apps.engine.models import (
ClientFile, Job, JobType, Label, AttributeSpec, SegmentType, Task, Project, Issue, Data,
)Would you like me to open a new GitHub issue to track this change, or will you handle it in this PR?
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Scan for unused imports in the specified Python file
flake8 --select=F401 cvat/apps/engine/views.py
Length of output: 122
Script:
#!/bin/bash
# List all import statements in the file
grep -E '^import |^from ' cvat/apps/engine/views.py > imports.txt
# Check usage of each imported module or symbol
while read -r line; do
# Extract the module or symbol name
symbol=$(echo "$line" | sed -E 's/^import |^from ([^ ]+) import .*/\1/')
# Check if the symbol is used in the file
if ! grep -q "$symbol" cvat/apps/engine/views.py; then
echo "Unused import: $line"
fi
done < imports.txt
Length of output: 11277
New Learnings Added
User: klakhov
PR: cvat-ai/cvat#7624
File: cvat/apps/engine/views.py:52-52
Timestamp: 2024-05-23T13:50:52.155Z
Learning: The import statement `import cvat.apps.dataset_manager.views # pylint: disable=unused-import` in `cvat/apps/engine/views.py` is unused and can be removed.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
There was a problem hiding this comment.
Remove unused import.
I have not imported this. This is here since 3 years ago.
Should i remove this??
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -2196,7 +2196,7 @@ class LabelViewSet(viewsets.GenericViewSet, mixins.ListModelMixin, | |||
| mixins.RetrieveModelMixin, mixins.DestroyModelMixin, PartialUpdateModelMixin | |||
| ): | |||
| queryset = Label.objects.prefetch_related( | |||
| 'attributespec_set', | |||
| Prefetch('attributespec_set', queryset=AttributeSpec.objects.order_by('display_order')), | |||
There was a problem hiding this comment.
Remove unnecessary f-string.
- f""
+ ""Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| Prefetch('attributespec_set', queryset=AttributeSpec.objects.order_by('display_order')), | |
| Prefetch('attributespec_set', queryset=AttributeSpec.objects.order_by('display_order')), |
| const arr: SerializedAttribute[] = []; // to store the updated attributes | ||
| const updatedLayout = this.formRef.current?.getFieldValue('layout'); | ||
| if (updatedLayout) { | ||
| const attr = values.attributes; | ||
| updatedLayout.forEach((obj: any, index: number) => { | ||
| arr[obj.y] = attr[index]; | ||
| }); | ||
| } | ||
| values.attributes = arr; |
There was a problem hiding this comment.
The logic for updating the attributes based on the new layout state is correct. Consider adding comments to explain the purpose of each step for better readability.
+ // Store the updated attributes based on the new layout
const arr: SerializedAttribute[] = [];
const updatedLayout = this.formRef.current?.getFieldValue('layout');
if (updatedLayout) {
const attr = values.attributes;
updatedLayout.forEach((obj: any, index: number) => {
arr[obj.y] = attr[index];
});
}
values.attributes = arr;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| const arr: SerializedAttribute[] = []; // to store the updated attributes | |
| const updatedLayout = this.formRef.current?.getFieldValue('layout'); | |
| if (updatedLayout) { | |
| const attr = values.attributes; | |
| updatedLayout.forEach((obj: any, index: number) => { | |
| arr[obj.y] = attr[index]; | |
| }); | |
| } | |
| values.attributes = arr; | |
| // Store the updated attributes based on the new layout | |
| const arr: SerializedAttribute[] = []; // to store the updated attributes | |
| const updatedLayout = this.formRef.current?.getFieldValue('layout'); | |
| if (updatedLayout) { | |
| const attr = values.attributes; | |
| updatedLayout.forEach((obj: any, index: number) => { | |
| arr[obj.y] = attr[index]; | |
| }); | |
| } | |
| values.attributes = arr; |
|
Do I still need updated_date as I am using display_order for the sorting field?? |
|
Any comments? Please, try to finalize the pull request this week. |
|
Hi @KrishavRajSingh , |
IMHO it is worthy of a dedicated pull request if we want these fields in Attribute and Label model. |
|
Alright, lets keep it for dedicated PR. @KrishavRajSingh, lets remove updated_date for now. |
Removed it, you may review it. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
cvat/apps/engine/serializers.py (2)
Line range hint
711-711: Remove unnecessary f-string.- raise exceptions.NotFound(f'Attribute with id #{attr_id} does not exist') + raise exceptions.NotFound('Attribute with id #{} does not exist'.format(attr_id))
Line range hint
1146-1146: Remove unnecessary f-string.- raise serializers.ValidationError(f'The specified project #{project_id} does not exist.') + raise serializers.ValidationError('The specified project #{} does not exist.'.format(project_id))
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- cvat-ui/src/components/labels-editor/label-form.tsx (7 hunks)
- cvat/apps/engine/migrations/0082_remove_attributespec_updated_time.py (1 hunks)
- cvat/apps/engine/models.py (1 hunks)
- cvat/apps/engine/serializers.py (4 hunks)
Files skipped from review due to trivial changes (1)
- cvat/apps/engine/migrations/0082_remove_attributespec_updated_time.py
Files skipped from review as they are similar to previous changes (2)
- cvat-ui/src/components/labels-editor/label-form.tsx
- cvat/apps/engine/models.py
Additional Context Used
Ruff (2)
cvat/apps/engine/serializers.py (2)
711-711: f-string without any placeholders
1146-1146: f-string without any placeholders
Additional comments not posted (4)
cvat/apps/engine/serializers.py (4)
432-432: Ensure thatdisplay_orderis updated correctly during attribute updates.This line correctly sets the
display_orderfor each attribute during the update process, ensuring that the order is maintained as per user interaction.
438-438: Incrementdisplay_orderto maintain the correct sequence.This line correctly increments the
display_orderafter updating each attribute, ensuring that the order is sequential and consistent.
492-492: Setdisplay_orderduring attribute creation to maintain order.This line correctly initializes the
display_orderfor new attributes, ensuring that the order is maintained from the creation phase.
494-494: Incrementdisplay_orderafter creating each attribute to maintain the correct sequence.This line correctly increments the
display_orderafter creating each attribute, ensuring that the order is sequential and consistent.
|
@KrishavRajSingh , You need to update the shema |
Done |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- cvat/schema.yml (3 hunks)
Additional comments not posted (3)
cvat/schema.yml (3)
4-4: API version updated to reflect new features.
6333-6334: Addeddisplay_orderfield to schema to support attribute reordering.
6361-6362: Consistent addition ofdisplay_orderacross different schema sections.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
cvat/apps/engine/serializers.py (2)
Line range hint
421-450: Ensuredisplay_orderis initialized correctly to support incremental attribute additions.- display_order = 0 + display_order = models.AttributeSpec.objects.filter(label=db_label).aggregate(Max('display_order'))['display_order__max'] or 0
Line range hint
725-725: Replace f-strings with regular strings when there are no placeholders.- f'label:update Skeleton id:{db_skeleton.id} for label_id:{db_label.id}' + 'label:update Skeleton id:{} for label_id:{}'.format(db_skeleton.id, db_label.id) - f'label:create Skeleton id:{db_skeleton.id} for label_id:{db_label.id}' + 'label:create Skeleton id:{} for label_id:{}'.format(db_skeleton.id, db_label.id)Also applies to: 1160-1160
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat/apps/engine/serializers.py (4 hunks)
- cvat/schema.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat/schema.yml
Additional Context Used
Ruff (2)
cvat/apps/engine/serializers.py (2)
725-725: f-string without any placeholders
1160-1160: f-string without any placeholders
| @@ -235,10 +235,11 @@ class AttributeSerializer(serializers.ModelSerializer): | |||
| values = DelimitedStringListField(allow_empty=True, | |||
| child=serializers.CharField(allow_blank=True, max_length=200), | |||
| ) | |||
| display_order = serializers.IntegerField(required=False) | |||
There was a problem hiding this comment.
Consider adding a default value for display_order to ensure consistent behavior when not explicitly set.
- display_order = serializers.IntegerField(required=False)
+ display_order = serializers.IntegerField(required=False, default=0)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| display_order = serializers.IntegerField(required=False) | |
| display_order = serializers.IntegerField(required=False, default=0) |
| display_order = 0 | ||
| for attr in attributes: | ||
| if attr.get('id', None): | ||
| del attr['id'] | ||
|
|
||
| attr['display_order'] = display_order | ||
| models.AttributeSpec.objects.create(label=db_label, **attr) | ||
| display_order += 1 |
There was a problem hiding this comment.
Initialize display_order based on existing attributes to support incremental additions.
- display_order = 0
+ display_order = models.AttributeSpec.objects.filter(label=db_label).aggregate(Max('display_order'))['display_order__max'] or 0Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| display_order = 0 | |
| for attr in attributes: | |
| if attr.get('id', None): | |
| del attr['id'] | |
| attr['display_order'] = display_order | |
| models.AttributeSpec.objects.create(label=db_label, **attr) | |
| display_order += 1 | |
| display_order = models.AttributeSpec.objects.filter(label=db_label).aggregate(Max('display_order'))['display_order__max'] or 0 | |
| for attr in attributes: | |
| if attr.get('id', None): | |
| del attr['id'] | |
| attr['display_order'] = display_order | |
| models.AttributeSpec.objects.create(label=db_label, **attr) | |
| display_order += 1 |
|
Small problem, after I change the order the drag icon becomes inactive and I cant drag once again: |
| @@ -49,7 +49,7 @@ | |||
| from cvat.apps.engine.frame_provider import FrameProvider | |||
| from cvat.apps.engine.media_extractors import get_mime | |||
| from cvat.apps.engine.models import ( | |||
| ClientFile, Job, JobType, Label, SegmentType, Task, Project, Issue, Data, | |||
| ClientFile, Job, JobType, Label, AttributeSpec, SegmentType, Task, Project, Issue, Data, | |||
| @@ -417,6 +418,7 @@ def update_label( | |||
| except models.InvalidLabel as exc: | |||
| raise exceptions.ValidationError(str(exc)) from exc | |||
|
|
|||
| display_order = 0 | |||
There was a problem hiding this comment.
We can use default value in model instead of implicit 0
| } | ||
|
|
||
| .cvat-drag-drop { | ||
| cursor: pointer | ||
| } |
| @@ -63,6 +67,16 @@ export default class LabelForm extends React.Component<Props> { | |||
| const { | |||
| label, onSubmit, onSkeletonSubmit, onCancel, resetSkeleton, | |||
| } = this.props; | |||
| const arr: SerializedAttribute[] = []; // to store the updated attributes | |||
There was a problem hiding this comment.
Lets use more descriptive variable name instead of comment
| @@ -63,6 +67,16 @@ export default class LabelForm extends React.Component<Props> { | |||
| const { | |||
| label, onSubmit, onSkeletonSubmit, onCancel, resetSkeleton, | |||
| } = this.props; | |||
| const arr: SerializedAttribute[] = []; // to store the updated attributes | |||
| const updatedLayout = this.formRef.current?.getFieldValue('layout'); | |||
There was a problem hiding this comment.
Why do we store layout in the form, but not in the state of the component?
We dont need to send layout to the server as its in the attributes fields
|
Also padding after attribute list became too small. Previously we had: |
|
Can we disable vertical resizing too? Horizontal is disabled as I can see |
|
Also please check failing linters and tests |
There was a problem hiding this comment.
Actionable comments posted: 1
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cvat-ui/src/components/labels-editor/label-form.tsx (7 hunks)
- cvat-ui/src/components/labels-editor/styles.scss (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- cvat-ui/src/components/labels-editor/styles.scss
Additional Context Used
Biome (20)
cvat-ui/src/components/labels-editor/label-form.tsx (20)
75-75: Unexpected any. Specify a different type.
157-157: Unexpected any. Specify a different type.
163-163: Unexpected any. Specify a different type.
163-163: Unexpected any. Specify a different type.
166-166: Unexpected any. Specify a different type.
166-166: Unexpected any. Specify a different type.
182-182: Unexpected any. Specify a different type.
196-196: Unexpected any. Specify a different type.
196-196: Unexpected any. Specify a different type.
241-241: Unexpected any. Specify a different type.
241-241: Unexpected any. Specify a different type.
246-246: Unexpected any. Specify a different type.
323-323: Unexpected any. Specify a different type.
345-345: Unexpected any. Specify a different type.
345-345: Unexpected any. Specify a different type.
349-349: Unexpected any. Specify a different type.
398-398: Unexpected any. Specify a different type.
408-408: Unexpected any. Specify a different type.
408-408: Unexpected any. Specify a different type.
426-426: Unexpected any. Specify a different type.
Additional comments not posted (4)
cvat-ui/src/components/labels-editor/label-form.tsx (4)
Line range hint
20-31: Import and setup ofreact-grid-layoutfor drag-and-drop functionality.This setup is crucial for the drag-and-drop feature and has been correctly implemented. The
WidthProviderHOC is used to manage the grid's width, which is a common practice withreact-grid-layout.
465-501: Implementation of the drag-and-drop UI for attributes.The UI components for drag-and-drop are well-structured and follow the design requirements. The use of
HolderOutlinedfor the drag handle is consistent with the initial feedback from the review comments.
704-730: Integration ofReactGridLayoutfor managing attribute layout.The integration of
ReactGridLayoutto manage the layout of attributes dynamically is a good use of the library. TheonLayoutChangehandler updates the form's layout state correctly, ensuring that the attribute order is preserved.
70-79: Handling of attribute reordering logic in the form submission.The logic to reorder attributes based on the layout from the form is well-implemented. However, consider using a more specific type than
anyfor theobjparameter in the loop to enhance type safety.
| if (updatedLayout) { | ||
| // Updating attributes based on updatedLayout before sending it to server | ||
| const attr = values.attributes; | ||
| updatedLayout.forEach((obj: any, index: number) => { |
There was a problem hiding this comment.
Use a more specific type than any.
Using any can lead to potential runtime errors due to lack of type safety. Consider defining a more specific type for the obj parameter.
- updatedLayout.forEach((obj: any, index: number) => {
+ updatedLayout.forEach((obj: LayoutObj, index: number) => {Committable suggestion was skipped due low confidence.
I have fixed almost everything, but I am not able to run the tests locally. Please help me with that. |
|
@KrishavRajSingh |
Added the feature of dragging attributes
Motivation and context
resolves #7370
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
Style