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

Drag'n'drop added/existing file entries, add empty folder #3999

Merged
merged 4 commits into from
Jun 19, 2024

Conversation

fiskus
Copy link
Member

@fiskus fiskus commented Jun 12, 2024

  • Moved FilesState to PackageDialog/FilesState.ts, so I can easily write tests
    • also, moved getNormalizedPath because it was used only for FilesState

Add empty folder:

  • Added FilesState.AddFolder
  • Added "Add new folder" button
  • For creating empty folder, I virtually add ".quiltkeep" file. Then I don't insert this file in UI, and ignore it when submit

Drag'n'Drop:

  • Added FilesState.Move({ source: string, dest: string }) for added and existing files and directories (so, 4 types of source, dest is always an added directory)
  • To have more control, I added draggable handler for file/dir icons only
  • Added DndProveder for managing state: what file do we drag, above what directory, and handle drop.
simplescreenrecorder-2024-06-17_16.23.48.webm
  • Changelog entry (skip if change is not significant to end users, e.g. docs only)

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 46.03175% with 136 lines in your changes missing coverage. Please review.

Project coverage is 37.33%. Comparing base (3e55c58) to head (705bf28).

Files Patch % Lines
...app/containers/Bucket/PackageDialog/FilesInput.tsx 0.00% 120 Missing ⚠️
catalog/app/components/Dialog/Prompt.tsx 0.00% 12 Missing ⚠️
.../app/containers/Bucket/PackageDialog/FilesState.ts 98.30% 2 Missing ⚠️
...iners/Bucket/PackageDialog/PackageCreationForm.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3999      +/-   ##
==========================================
+ Coverage   37.19%   37.33%   +0.14%     
==========================================
  Files         757      758       +1     
  Lines       33135    33321     +186     
  Branches     4854     4903      +49     
==========================================
+ Hits        12323    12442     +119     
- Misses      19652    20235     +583     
+ Partials     1160      644     -516     
Flag Coverage Δ
api-python 90.75% <ø> (ø)
catalog 11.01% <46.03%> (+0.44%) ⬆️
lambda 88.18% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fiskus fiskus marked this pull request as ready for review June 17, 2024 14:17
@fiskus fiskus requested a review from nl0 June 17, 2024 14:27
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

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

well, this looks cleaner than i expected. special thanks for extracting the FilesState code, cleaning it up and covering with tests.

i haven't tested the actual ux tho

@fiskus fiskus requested a review from nl0 June 18, 2024 17:50
Copy link
Member

@nl0 nl0 left a comment

Choose a reason for hiding this comment

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

looks good

@fiskus
Copy link
Member Author

fiskus commented Jun 19, 2024

  • Left only that "Add folder" icon on the top. And swap with "Undo changes"
  • Enabled drag'n'drop only for "added", "modified" or existing "unchanged" files and directories
  • Squashed commits

@fiskus fiskus requested a review from nl0 June 19, 2024 09:31
@nl0
Copy link
Member

nl0 commented Jun 19, 2024

Squashed commits

i don't think it's a good idea tbh, because it makes incremental review hard.
commits are automatically squashed when the PR is merged anyways.

nl0
nl0 previously approved these changes Jun 19, 2024
docs/CHANGELOG.md Outdated Show resolved Hide resolved
@fiskus
Copy link
Member Author

fiskus commented Jun 19, 2024

commits are automatically squashed when the PR is merged anyways.

I didn't know. Last time I checked, they didn't

Co-authored-by: Alexei Mochalov <[email protected]>
@nl0
Copy link
Member

nl0 commented Jun 19, 2024

commits are automatically squashed when the PR is merged anyways.

I didn't know. Last time I checked, they didn't

check out the network graph under the "insights" tab

docs/CHANGELOG.md Outdated Show resolved Hide resolved
@fiskus fiskus enabled auto-merge June 19, 2024 10:07
@fiskus fiskus added this pull request to the merge queue Jun 19, 2024
@fiskus
Copy link
Member Author

fiskus commented Jun 19, 2024

I mean: yes, commits are squashed, but messages were just concatenated earlier. I didn't notice when it was changed

Merged via the queue into master with commit 15fcd5a Jun 19, 2024
37 of 38 checks passed
@fiskus fiskus deleted the package-directory branch June 19, 2024 10:16
@nl0
Copy link
Member

nl0 commented Jun 19, 2024

I mean: yes, commits are squashed, but messages were just concatenated earlier. I didn't notice when it was changed

oh i see what you mean. we tweaked that setting some time ago, yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants