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

UI update a file #63

Merged
merged 9 commits into from
Jul 5, 2023
Merged

UI update a file #63

merged 9 commits into from
Jul 5, 2023

Conversation

hans-hamel
Copy link
Collaborator

@hans-hamel hans-hamel commented Jul 3, 2023

Depends on PR #61. Once the 'state/updateNonGms' branch is merged, change the base base branch to 'develop'.

Implement the UI to update a file.

ui-upload-file.mov

@hans-hamel hans-hamel added Don't merge Can't be merged yet Depends on This have dependency with another PR labels Jul 3, 2023
@hans-hamel hans-hamel self-assigned this Jul 3, 2023
Copy link
Contributor

@HectorNarvaez HectorNarvaez left a comment

Choose a reason for hiding this comment

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

added some comments

@hans-hamel hans-hamel force-pushed the state/uiUpdateFile branch 2 times, most recently from 4f8e7bb to addb88f Compare July 4, 2023 22:17
Base automatically changed from state/updateNonGms to develop July 4, 2023 22:21
@HectorNarvaez HectorNarvaez removed Don't merge Can't be merged yet Depends on This have dependency with another PR labels Jul 4, 2023
Copy link
Contributor

@HectorNarvaez HectorNarvaez left a comment

Choose a reason for hiding this comment

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

As discussed yesterday via teams, currently this doesn't have the expected behavior. Waiting for new changes

Copy link
Contributor

@HectorNarvaez HectorNarvaez left a comment

Choose a reason for hiding this comment

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

Added some comments

@@ -55,8 +55,10 @@ class FileViewerViewModel @Inject constructor(
is FileViewerViewEvent.CreateFile -> createFileEvent(event)
is FileViewerViewEvent.DeleteFile -> deleteFileEvent(event)
is FileViewerViewEvent.UploadFile -> uploadFile(event)
is FileViewerViewEvent.UpdateFile -> updateFile(event)
Copy link
Contributor

Choose a reason for hiding this comment

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

please move this below is FileViewerViewEvent.UpdateFileClicked -> updateFileClicked(event) (line 61)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why is neccesary to move below?

@@ -110,14 +112,14 @@ class FileViewerViewModel @Inject constructor(
}

private fun downloadFileEvent() {
setState(FileViewerViewState.Loading)
Copy link
Contributor

Choose a reason for hiding this comment

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

why we moved this?

user should see a loading screen once we start to execute any logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because if the file is not downloadable, the file will not be downloadable then the loading state won't be dismissed

Copy link
Contributor

Choose a reason for hiding this comment

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

we can dismiss it dispatching a refresh event:

setState(FileViewerViewState.Loading)

if (!file.isDownloadable()) {
            toastMessage.postValue("${file.name} is not downloadable")
            refreshFileListEvent()
            return
}

private fun updateFile(event: FileViewerViewEvent.UpdateFile) {
val fileId = lastFileClicked?.id ?: return

setState(FileViewerViewState.Loading)
Copy link
Contributor

Choose a reason for hiding this comment

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

please move this below method declaration

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you move below the method declaration, the state loading will be there if the lastFileClicked is null

Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, call refresh method

@@ -137,6 +139,35 @@ class FileViewerViewModel @Inject constructor(
}
}

private fun updateFile(event: FileViewerViewEvent.UpdateFile) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please rename this as updateFileEvent

@HectorNarvaez HectorNarvaez self-requested a review July 5, 2023 23:29
Copy link
Contributor

@HectorNarvaez HectorNarvaez left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -110,14 +112,14 @@ class FileViewerViewModel @Inject constructor(
}

private fun downloadFileEvent() {
setState(FileViewerViewState.Loading)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can dismiss it dispatching a refresh event:

setState(FileViewerViewState.Loading)

if (!file.isDownloadable()) {
            toastMessage.postValue("${file.name} is not downloadable")
            refreshFileListEvent()
            return
}

private fun updateFile(event: FileViewerViewEvent.UpdateFile) {
val fileId = lastFileClicked?.id ?: return

setState(FileViewerViewState.Loading)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above, call refresh method

@HectorNarvaez HectorNarvaez merged commit 40b643f into develop Jul 5, 2023
2 checks passed
@HectorNarvaez HectorNarvaez deleted the state/uiUpdateFile branch July 5, 2023 23:33
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.

None yet

3 participants