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

feat: Handle file upload from Flagship #2969

Merged
merged 17 commits into from
Oct 23, 2023

Conversation

acezard
Copy link
Contributor

@acezard acezard commented Aug 23, 2023

  • Upload one file
  • Upload multiple files
  • Handle upload progression without leaving the app with one file
  • Handle upload progression without leaving the app with multiple files
  • Handle upload progression after leaving and opening the app again with one file
  • Handle upload progression after leaving and opening the app again with multiple files

@acezard acezard force-pushed the refactor--updated-upload-component-to-typescript branch 4 times, most recently from 569db51 to 5a3eb17 Compare August 24, 2023 09:40
@acezard acezard changed the title refactor: updated upload component to typescript feat: Handle file upload from Flagship Aug 30, 2023
@acezard acezard force-pushed the refactor--updated-upload-component-to-typescript branch from 7bd34e8 to ec80628 Compare September 3, 2023 16:30
@acezard acezard changed the base branch from feat--Update-manifest-to-add-acceptFromFlagship to master September 4, 2023 07:01
@acezard acezard marked this pull request as ready for review September 4, 2023 07:01
@acezard acezard force-pushed the refactor--updated-upload-component-to-typescript branch from 7ce7c11 to 880b97e Compare September 4, 2023 07:08
title={t('ImportToDrive.title', { smart_count: items.length })}
subTitle={t('ImportToDrive.to')}
/>
<Query
Copy link
Contributor

Choose a reason for hiding this comment

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

Query component is deprecated. You should have refactor to useQuery(). I still thin that this kind of refactor to be up to date with the cozy latest standard should be done BEFORE converting to TS...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored

@Crash--
Copy link
Contributor

Crash-- commented Sep 12, 2023

I have to delete the cache on travis to be able to build this branch. And locally I have to rm -rf node_modules/ ....

@bundlemon
Copy link

bundlemon bot commented Sep 12, 2023

BundleMon

Files updated (6)
Status Path Size Limits
drive/manifest.webapp
1.88KB (+114B +6.28%) 2KB
drive/services/qualificationMigration/drive.j
s
250.63KB (-680B -0.26%) 500KB
drive/services/dacc/drive.js
255.46KB (-1.08KB -0.42%) 500KB
drive/intents/drive.(hash).js
156.65KB (-7.59KB -4.62%) 171KB
drive/public/drive.(hash).js
1.52MB (-18.91KB -1.2%) 1.65MB
drive/app/drive.(hash).js
247.84KB (-23.46KB -8.65%) 280KB
Unchanged files (13)
Status Path Size Limits
drive/vendors/drive.(hash).js
1.3MB 1.6MB
drive/public/pdf.worker.entry.(hash).worker.j
s
343.37KB 345KB
drive/public/cozy-client-js.js
158.97KB 160KB
drive/public/drive.(hash).min.css
92.6KB 100KB
drive/app-drive.(hash).min.css
55.72KB 56KB
drive/intents-drive.(hash).min.css
37.06KB 40KB
drive/onlyOffice/slide.pptx
24.82KB 25KB
drive/onlyOffice/text.docx
5.84KB 6KB
drive/onlyOffice/spreadsheet.xlsx
5.01KB 6KB
drive/index.html
532B 1KB
drive/intents/index.html
516B 1KB
drive/img/app-icon.(hash).svg
419B 1KB
drive/manifest.json
184B 1KB

Total files change -51.6KB -1.13%

Groups updated (4)
Status Path Size Limits
drive/services/**
506.08KB (-1.75KB -0.34%) +1%
drive/intents/**
157.15KB (-7.59KB -4.61%) +6%
drive/public/**
2.11MB (-18.91KB -0.87%) +7%
drive/app/**
247.84KB (-23.46KB -8.65%) +10%
Unchanged groups (4)
Status Path Size Limits
drive/screenshots/**
2.13MB +0.4%
drive/vendors/**
1.3MB +6%
drive/onlyOffice/**
35.68KB +0.4%
drive/img/**
419B +0.4%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

package.json Outdated Show resolved Hide resolved
src/components/App/App.jsx Outdated Show resolved Hide resolved
}, [dispatch, removeFileToUploadQueue, t])

const onClose = useCallback(async () => {
await webviewIntent?.call('resetFilesToHandle').then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still really don't like this way of writing code... Can we do it differently to be the same as other place?

- Also add placeholder route that will handle mobile uploads later
It is believed this will help integrating with the
existing store logic.
No runtime changes are intended, only typings so everything is
as clear as possible when reusing the already existing mobile upload
flow
has to be put at a better location
@acezard acezard force-pushed the refactor--updated-upload-component-to-typescript branch 4 times, most recently from 1331b4b to 76634d1 Compare October 17, 2023 13:26
this commit:
- removes modification in upload module
- removes logic in uploaderView
- regroup logic and state management in the custom hook of the feature
- removes deprecated Query component
- enhances UI loading state
- accept 10 files maximum
@acezard acezard force-pushed the refactor--updated-upload-component-to-typescript branch from 76634d1 to 96ce541 Compare October 17, 2023 14:05
@acezard acezard requested a review from Crash-- October 17, 2023 14:05
Also remove old import from a test file.
The test file itself seems a bit outdated,
it would be good to review it at some point
@acezard acezard requested a review from Ldoppea October 19, 2023 13:15

logger('info', 'uploadFilesFromFlagship called')

await webviewIntent?.call('uploadFiles', JSON.stringify({ fileOptions }))
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that we pass only one file at a time to uploadFiles. Didn't we want to pass an array here?

If yes we should modify the interface.

If no we should rename this as uploadFile.

Copy link
Contributor Author

@acezard acezard Oct 20, 2023

Choose a reason for hiding this comment

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

Yes it's one by one, it was made like that to mimick the old upload to mobile feature. In this case renaming seems the best approach

done on react-native side too cozy/cozy-flagship-app@5278d76

Instead of removing all files to upload,
we go back to the selection screen
@acezard acezard requested a review from Ldoppea October 20, 2023 06:55
@acezard acezard merged commit 0259f69 into master Oct 23, 2023
3 checks passed
@acezard acezard deleted the refactor--updated-upload-component-to-typescript branch October 23, 2023 07:41
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.

3 participants