[FEATURE REQUEST] Allow to go to the destination folder when the copy/move operation is finished#4802
[FEATURE REQUEST] Allow to go to the destination folder when the copy/move operation is finished#4802mykh-hailo wants to merge 7 commits intoowncloud:masterfrom
Conversation
b088f36 to
9a416e0
Compare
|
Thanks for your contribution @mykh-hailo! 🍻 Just a small note before the CR: please be careful with the branch name. The branch prefixes we use in the project are: I will take care of this as soon as possible and I'll ping you when it's ready 😄 |
|
@joragua I'd appreciate it if you share any feedback on the result. |
|
@joragua Can you check my PR please? |
|
@mykh-hailo I'll ping you when the CR is finished. Stay tuned! 😄 |
|
@joragua Can you provide me any updates on this? |
|
Hi @mykh-hailo! I'm working on the PR but it will not be available until next week. I'll ping you once it's done. No worries! 😄 Thanks for your patience! |
joragua
left a comment
There was a problem hiding this comment.
Nice job @mykh-hailo! 🙌🏻 Somme comments here.
NOTE: I'd add a release note since this is an important and useful improvement for end users. They should be aware of it this and see the new feature in the release notes screen. You can add a new entry in ReleaseNotesViewModel.kt with an appropriate title and subtitle. Let us know if you have any question about this process. 😄
owncloudApp/src/main/java/com/owncloud/android/extensions/ActivityExt.kt
Show resolved
Hide resolved
| } else { | ||
| showMessageInSnackbar(R.id.list_layout, message) | ||
| } | ||
| copyMoveTargetFolder = null |
There was a problem hiding this comment.
Just a question: Is it necessary to set null at the end? Since the copyMoveTargetFolder value is set at the begin of requestMoveOperation and copyMoveOperation methods
There was a problem hiding this comment.
Just a question: Is it necessary to set
nullat the end? Since thecopyMoveTargetFoldervalue is set at the begin ofrequestMoveOperationandcopyMoveOperationmethods
What about this? 🤔
d886716 to
a2d1b92
Compare
|
@joragua Thank you for your feedback. |
joragua
left a comment
There was a problem hiding this comment.
Some comments and it's ready @mykh-hailo! 😄
-
We usually use
refactor:prefix in commits when we change the code but keep the same behavior (string changes, method refactoring...) We use the prefixfix:when something is broken and we must fix it. My suggestion:refactor: update string resource -
The prefix used for release note is
feat:since it's a new improvement in the app that end users can see. My suggestion:feat: add release note
You can do an interactive rebase and change the name of the commits applying my suggestions, in order to follow the project convention for commits. Let us know if you have any question about the process 🍻
| showSnackbarWithAction( | ||
| message = getString(R.string.sync_fail_ticker_unauthorized), | ||
| actionText = getString(R.string.auth_oauth_failure_snackbar_action), | ||
| action = { |
There was a problem hiding this comment.
I think you removed the snackbar duration from here, and it should be displayed indefinite: duration = Snackbar.LENGTH_INDEFINITE
| } else { | ||
| showMessageInSnackbar(R.id.list_layout, message) | ||
| } | ||
| copyMoveTargetFolder = null |
There was a problem hiding this comment.
Just a question: Is it necessary to set
nullat the end? Since thecopyMoveTargetFoldervalue is set at the begin ofrequestMoveOperationandcopyMoveOperationmethods
What about this? 🤔
| <string name="move_file_invalid_into_descendent">It is not possible to move a folder into a descendant.</string> | ||
| <string name="move_file_invalid_overwrite">The file exists already in the destination folder.</string> | ||
| <string name="move_file_error">An error occurred while trying to move this file or folder.</string> | ||
| <string name="move_success_msg">File moved correctly</string> |
There was a problem hiding this comment.
| <string name="move_success_msg">File moved correctly</string> | |
| <string name="move_file_correctly">File moved correctly</string> |
| <string name="copy_file_invalid_into_descendent">It is not possible to copy a folder into a descendant.</string> | ||
| <string name="copy_file_invalid_overwrite">The file exists already in the destination folder.</string> | ||
| <string name="copy_file_error">An error occurred while trying to copy this file or folder.</string> | ||
| <string name="copy_success_msg">File copied correctly</string> |
There was a problem hiding this comment.
| <string name="copy_success_msg">File copied correctly</string> | |
| <string name="copy_file_correctly">File copied correctly</string> |
| <string name="release_notes_4_8_0_title_action_to_copy_or_move_destination_folder">Action to copy/move destination folder</string> | ||
| <string name="release_notes_4_8_0_subtitle_action_to_copy_or_move_destination_folder">Added a snackbar action to quickly navigate to the destination folder after copy/move operations.</string> |
There was a problem hiding this comment.
We don't usually include technical words in the release notes such as snackbar.
My suggesstion:
| <string name="release_notes_4_8_0_title_action_to_copy_or_move_destination_folder">Action to copy/move destination folder</string> | |
| <string name="release_notes_4_8_0_subtitle_action_to_copy_or_move_destination_folder">Added a snackbar action to quickly navigate to the destination folder after copy/move operations.</string> | |
| <string name="release_notes_4_8_0_title_action_to_copy_or_move_destination_folder">Navigate to destination folder</string> | |
| <string name="release_notes_4_8_0_subtitle_action_to_copy_or_move_destination_folder">New action to navigate to the destination folder when a file is copied or moved</string> |
Do you have any idea for the release note @jesmrec? (open to discussion)
There was a problem hiding this comment.
That's right. Release notes target any kind of user, so, we can not assume that technical words are understood by everyone. Better to use basic language. About your approach, @joragua, it's ok with tiny changes from my side #
| <string name="release_notes_4_8_0_title_action_to_copy_or_move_destination_folder">Action to copy/move destination folder</string> | |
| <string name="release_notes_4_8_0_subtitle_action_to_copy_or_move_destination_folder">Added a snackbar action to quickly navigate to the destination folder after copy/move operations.</string> | |
| <string name="release_notes_4_8_0_title_action_to_copy_or_move_destination_folder">Navigation to target folder</string> | |
| <string name="release_notes_4_8_0_subtitle_action_to_copy_or_move_destination_folder">New action to navigate to the destination folder when a file or folder is copied or moved<</string> |
There was a problem hiding this comment.
Nice! @mykh-hailo I'd go for the release note approach that @jesmrec commented 🙌🏻
a2d1b92 to
c394298
Compare
c394298 to
2468ae7
Compare
Related Issues
App: #4379
ReleaseNotesViewModel.ktcreating a newReleaseNote()with String resources (if required)