Moves upload component to mobX #1732
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
packages/odf/components/s3-browser/objects-list/ObjectListWithSidebar.tsx
Outdated
Show resolved
Hide resolved
packages/odf/components/s3-browser/upload-objects/upload-status/UploadStatusBasedAlert.tsx
Outdated
Show resolved
Hide resolved
| const { t } = useCustomTranslation(); | ||
| const [isModalOpen, setModalOpen] = React.useState(false); | ||
| export const AbortUploadsModal: React.FC<AbortUploadsModalProps> = observer( | ||
| ({ abortAll }) => { |
There was a problem hiding this comment.
same here, observer might not required here: https://mobx.js.org/react-integration.html#tip-grab-values-from-objects-as-late-as-possible
packages/odf/components/s3-browser/objects-list/ObjectListWithSidebar.tsx
Outdated
Show resolved
Hide resolved
| // eslint-disable-next-line guard-for-in | ||
| for (const key in this.uploads) { | ||
| delete this.uploads[key]; | ||
| } |
There was a problem hiding this comment.
what if ??
| // eslint-disable-next-line guard-for-in | |
| for (const key in this.uploads) { | |
| delete this.uploads[key]; | |
| } | |
| this.uploads = {}; |
There was a problem hiding this comment.
missed this comment @bipuladh.
packages/odf/components/s3-browser/upload-objects/upload-component/FileUploadComponent.tsx
Outdated
Show resolved
Hide resolved
packages/odf/components/s3-browser/objects-list/ObjectListWithSidebar.tsx
Outdated
Show resolved
Hide resolved
packages/odf/components/s3-browser/upload-objects/UploadSidebar.tsx
Outdated
Show resolved
Hide resolved
bipuladh
force-pushed
the
mobx-switch
branch
2 times, most recently
from
fb4e862
to
749f591
Compare
bipuladh
changed the title
bipuladh
requested review from
SanjalKatiyar,
alfonsomthd and
GowthamShanmugam
Signed-off-by: Bipul Adhikari <badhikar@redhat.com>
| showSidebar={showUploadSidebar} | ||
| abortAll={abortAll} | ||
| hideSidebar={hideUploadSidebar} |
There was a problem hiding this comment.
can't we simply use closeUploadSidebar ??
| hideSidebar={hideUploadSidebar} | |
| hideSidebar={closeUploadSidebar} |
| </Drawer> | ||
| ); | ||
| }; | ||
| export const UploadSidebar: React.FC<UploadSidebarProps> = observer( |
There was a problem hiding this comment.
is observer needed here ?? I don't see store being used ??
| // eslint-disable-next-line guard-for-in | ||
| for (const key in this.uploads) { | ||
| delete this.uploads[key]; | ||
| } |
There was a problem hiding this comment.
missed this comment @bipuladh.
| setAborter(key: string, aborter: UploadProgress['abort']) { | ||
| this.uploads[key]['abort'] = aborter; | ||
| } | ||
|
|
||
| // Add a file to the map | ||
| addFile(file: UploadProgress, key: string) { | ||
| this.uploads[key] = { | ||
| ...file, | ||
| uploadState: UploadStatus.INIT_STATE, | ||
| }; | ||
| } |
There was a problem hiding this comment.
just a question @bipuladh, why are we not using set keyword here with these functions ?? these also update observable state, so don't we need set keyword here too ?? what's is the criteria of using set ??
There was a problem hiding this comment.
set keyword is essentially a JS class concept and it doesn't support multiple parameters.
| foldersPath, | ||
| uploadStore | ||
| ); | ||
| setCompletionTime(completionTime); |
There was a problem hiding this comment.
earlier we were calling processFiles once, but now we are doing so in batches, so setCompletionTime(completionTime) will be called multiple times...
There was a problem hiding this comment.
ideally it should only be called once after all batches are complete...
There was a problem hiding this comment.
The value is used only when the total upload is completed but I can remove it from the loop. Thanks.
| loaded: 0, | ||
| uploadState: UploadStatus.INIT_STATE, | ||
| abort: null, | ||
| name: file.name, | ||
| filePath: file.webkitRelativePath, | ||
| startTime: undefined, | ||
| lastModified: 0, |
There was a problem hiding this comment.
lastModified is not used anywhere, right ??
|
|
||
| const getProgressVariant = ( | ||
| state: UploadStatus, | ||
| // For cases whjen it's complete but user presses cancel(edge case) |
There was a problem hiding this comment.
| // For cases whjen it's complete but user presses cancel(edge case) | |
| // For cases when it's complete but user presses cancel(edge case) |
| isComplete: boolean | ||
| ): Progress['props']['variant'] => { | ||
| if (isComplete) { | ||
| return undefined; |
There was a problem hiding this comment.
if upload is complete shouldn't we show success ??
| return undefined; | |
| return ProgressVariant.success; |
There was a problem hiding this comment.
makes sense. Thanks!
| const existingData = this.uploads[fileId]; | ||
|
|
||
| const update = | ||
| existingData.uploadState === UploadStatus.UPLOAD_COMPLETE |
There was a problem hiding this comment.
??
| existingData.uploadState === UploadStatus.UPLOAD_COMPLETE | |
| existingData.uploadState !== UploadStatus.UPLOAD_COMPLETE |
No description provided.