add loading animation to upload doc modal#654
Conversation
|
Just to make sure, is this still a draft or are you looking for a review already? |
|
@andycwilliams this is still a draft, for sure. I should have just pinged you instead of requesting a review. |
| </DialogActions> | ||
| </FormControl> | ||
| </form> | ||
| {processing && <LoadingAnimation loadingItem={file.name} />} |
There was a problem hiding this comment.
Rather than it being condition within the FormSection component, I think it should be wrapping it as part of a ternary:
{ processing ? <LoadingAnimation /> : <FormSection>modal content here</FormSection> }
Screen.Recording.2024-06-26.at.12.54.50.AM.mov
There was a problem hiding this comment.
So it would fully replace all of the content with the animation?
I prefer to stay with the Figma design. I think it looks a bit nicer and is less jarring than taking over the whole modal.
There was a problem hiding this comment.
The previous animation that's dangling at the bottom of the modal does not seem any better either. Might want to use a different kind of animation if not the full content
There was a problem hiding this comment.
Maybe we could look at a solution where the size of the modal does not change, which is jarring, and the loading animation displays over top of the modal with the content greyed out or removed. I don't mind the content being removed as long as the size of the modal doesn't change.
There was a problem hiding this comment.
Maybe we could look at a solution where the size of the modal does not change, which is jarring, and the loading animation displays over top of the modal with the content greyed out or removed. I don't mind the content being removed as long as the size of the modal doesn't change.
We could try that. Or else the animation could temporarily replace one of the buttons (while retaining the size). Maybe "Upload File".
Thinking farther ahead, in #642 I mention possibly breaking down this whole process into several steps. That may or may not affect what we decide here.
There was a problem hiding this comment.
@andycwilliams @leekahung this is the clip I tried to post. I'm not sure why it wasn't working before.
Screen.Recording.2024-06-28.at.7.08.32.PM.mov
There was a problem hiding this comment.
Ah, well that's pretty different! I'll take a closer look tomorrow.
There was a problem hiding this comment.
@andycwilliams @leekahung this is the clip I tried to post. I'm not sure why it wasn't working before.
Screen.Recording.2024-06-28.at.7.08.32.PM.mov
I like this one 👍
There was a problem hiding this comment.
I don't know why, but it's still displaying the way it does in my screenshot. It's not appearing as an overlay in either desktop or mobile modes even after pulling the latest version of the branch.
There was a problem hiding this comment.
@andycwilliams Apologies - I did not push the changes I made to the code until just now.
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
Since this is still a draft I'm not doing a full on review, just adding a couple of thoughts.
Looking pretty nice though!
| {/* <LoadingAnimation loadingItem={'testfilename'} /> */} | ||
| </FormSection> | ||
| <Backdrop open={processing}> | ||
| <LoadingAnimation loadingItem="file name" /> |
There was a problem hiding this comment.
Shouldn't loadingItem="file name" still be loadingItem={file.name}?
| <Paper | ||
| elevation={2} | ||
| sx={{ | ||
| display: 'inline-block', | ||
| mx: '2px', | ||
| padding: '20px', | ||
| backgroundColor: 'background.tint' | ||
| }} | ||
| > |
There was a problem hiding this comment.
While this works for this animation, it consequently affects other uses of this animation as well, such as when loading contacts or documents.
The box is now gray rather than white. Not a huge deal or anything, just an unintended side effect.
Best practice would be to carve out an exception for this. However, I'd like to do #643, which would refactor animations a tad anyway.
|
Any updates @russfraze? |
|
Without any updates this PR might need to be closed or reassigned. |
|
@andycwilliams I forgot about this, but back on it. I passed an isModal prop to the LoadIngAnimation component so that the background tint color is only if the LoadingAnimation is being used in a modal. The rest of the time it remains white. |
|
Thanks for continuing work on this! Looks like this branch has not been merged with the latest version of Development so it's behind a bit. Could you update it, please? |
andycwilliams
left a comment
andycwilliams
left a comment
There was a problem hiding this comment.
Sorry for the delay.
Are you also getting this effect where "Enter Description" appears through the pop-up?
It's been a while since I last looked at this. Are we definitively decided to use a pop-up design rather than include the animation in the modal itself?
If we're to leave it as a pop-up, I think the width should not exceed the modal itself.
3 participants
This PR:
Resolves #(654)
Screenshots (if applicable):
Add any screenshots/videos here.
Additional Context (optional):
I've implemented this and you can see the loading animation for a split second when uploading a document. I uploaded a 12MB image and it still only takes a split second. I'm wondering if that is because I am using the local pod server?
I think this loading animation makes for a better user experience if the file is large enough to take a couple of seconds, for sure.
Future Steps/PRs Needed to Finish This Work (optional):
This needs further styling to match the Figma file.
Issues needing discussion/feedback (optional):
Currently, the loadingAnimation component delivers an indeterminant progress bar which is fine, but the Figma file shows a determinate progress bar with a percentage of the upload progress. Let me know if we intend to have a determinate progress bar here and I can look into how to make that work.