-
Notifications
You must be signed in to change notification settings - Fork 31
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
Refactor/neon/dialogs #998
Conversation
67804d5
to
fa1cacc
Compare
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 do not agree with the move to put app implementation specific dialogs into the framework. If we want to support this then it should be done through the interface abstractions.
I'm sorry, but I think you will need to revert large parts of the PR.
I guess you mean the folder dialogs? Can you please provide feedback to the design (as I think it changed for every dialog). |
fa1cacc
to
d432c7e
Compare
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.
9c9ea70
to
ca0bf3c
Compare
1f9fd39
to
280c64e
Compare
@provokateurin ping :) |
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.
Looks pretty good to me, although I still found some problems:
Opening the copy/move dialogs throws Bad state: Snapshot has neither data nor error
and nothing is shown (this might already be the case on the main branch as well, I rebased this branch locally before testing).
I checked the move/copy dialogs on the main branch and there they work, so it's definitely a regression of these changes. |
Btw can you maybe also fix #1220 since you are already touch that part of the code? |
Sure. I'll take a look |
280c64e
to
60cdf4a
Compare
just a rebase for now. |
3373d66
to
1adb575
Compare
64640f1
to
0307e80
Compare
0307e80
to
8770601
Compare
Tests LGTM, I'll give it another round of tests to check that everything works correctly. |
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.
From my testing:
- Moving a file or folder has the file or folder set as the initial path. For folders this makes no sense and for files even less. The parent dir should be the initial path.
- Instead of greying out the choose button when trying to move a directory into itself the whole entry should just be hidden in the listing. This completely avoids the problem.
- Copying a folder into itself should still be possible (even though it is a weird use-case).
Everything else worked flawlessly ❤️
Why? This sounds like it should just break |
It is not a problem because the original folder still exists. You are only creating new files, not moving which is deleting and re-adding them which requires the folder to still exist while it doesn't |
Trying to copy a folder into itself returns in a 409 and an error message being shown |
Ok, then not special handling 👍 |
…alog structure Signed-off-by: Nikolas Rimikis <[email protected]>
…s): make dialog adaptive Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
8770601
to
681529a
Compare
Signed-off-by: Nikolas Rimikis <[email protected]>
Signed-off-by: Nikolas Rimikis <[email protected]>
681529a
to
f46202f
Compare
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.
LGTM, I think some of the logic for showing modals and bottom sheets could be moved from files into the framework so it can be reused, but that can be done later when needed.
Fixes: #995
Fixes: #535
I moved the folder dialogs (create, rename, delete) into the framework as this can be reused in many clients.