feat: add a progression view for media backup #184
Conversation
mobile/src/actions/mediaBackup.js
Outdated
|
|
||
| export const startMediaUpload = () => ({ type: MEDIA_UPLOAD_START }) | ||
| export const endMediaUpload = () => ({ type: MEDIA_UPLOAD_END }) | ||
| export const successImageUpload = (image) => ({ type: IMAGE_UPLOAD_SUCCESS, id: image.id }) | ||
| export const successImageUpload = (media) => ({ type: IMAGE_UPLOAD_SUCCESS, id: media.id }) |
There was a problem hiding this comment.
why do you change (image) to (media)? (we have successImageUpload and IMAGE_UPLOAD_SUCCESS)
This is not blocking
There was a problem hiding this comment.
We don't backup images but photos. And soon we won't upload photos but photos and videos. So I think media is a good shell to address all the needs.
mobile/src/actions/mediaBackup.js
Outdated
| @@ -33,8 +45,11 @@ export const mediaBackup = (dir) => async (dispatch, getState) => { | |||
| let photos = await getFilteredPhotos() | |||
| const alreadyUploaded = getState().mobile.mediaBackup.uploaded | |||
| const dirID = await getDirID(dir) | |||
| const totalUpload = photos.length - alreadyUploaded.length | |||
There was a problem hiding this comment.
It's not good here.
I think you can filter photos like that:
photos = photos.filter(photo => !alreadyUploaded.includes(photo.id))
const totalUpload = photos.length
and remove if (!alreadyUploaded.includes(photo.id)) { line 51
There was a problem hiding this comment.
I needed to get the total of going to be uploaded photos.
I think that
photos.length - alreadyUploaded.length === photos.filter(photo => !alreadyUploaded.includes(photo.id)).lengthSo this remark, though good enough, is just a matter of refactoring.
And as this code is not covered by tests, I refused to refactor it (red ⇒ green ⇒ refactor).
There was a problem hiding this comment.
Precisely it's not the same.
If a user deletes already uploaded photos, they will count in alreadyUploaded but not in photos, which will distort the calculation.
There was a problem hiding this comment.
That is a very good remark. Please find 77a6a49 to fix this.
mobile/src/reducers/mediaBackup.js
Outdated
| case IMAGE_UPLOAD_SUCCESS: | ||
| return { ...state, uploaded: [...state.uploaded, action.id] } | ||
| return { ...state, uploaded: [...state.uploaded, action.id], currentUpload: undefined } |
There was a problem hiding this comment.
if you do that (currentUpload: undefined) for each photo, the UploadProgression component will blinking...
There was a problem hiding this comment.
Good remarks, it is an oversight.
|
@kosssi did you have something else to add or do you approve this PR? I added a spinner
|
| @@ -27,21 +39,25 @@ async function getDirID (dir) { | |||
|
|
|||
| export const mediaBackup = (dir) => async (dispatch, getState) => { | |||
| if (backupAllowed(getState().mobile.settings.wifiOnly)) { | |||
| let photos = await getFilteredPhotos() | |||
| const photosOnDevice = await getFilteredPhotos() | |||
|
Alright |
We could use
Alerterfrom cozy-ui but it seems not correct for this kind of progression message (see @goldoraf and @gregorylegarec for more information).So the
UploadProgressioncomponent is the very same UI than an Alerter, but it did not have the same behavior, it is very simpler: if a message is display, it is shown, otherwise it is not.Hope it fits the needs.