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(filesLimit): handle if user goes over size limit #1715

Merged
merged 11 commits into from
Mar 4, 2022
Merged

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented Feb 25, 2022

What this PR does πŸ“–

  • add getter to calculate the total size of all tracked files
  • update files aside to track your files usage (amount and progress bar)
  • prevent further uploads if you're out of room

Which issue(s) this PR fixes πŸ”¨
AP-899, AP-910

Special notes for reviewers πŸ—’οΈ

  • Originally wanted to use a computed for the progress bar, but it would cause an infinite render issue. opted for watch instead

Additional comments 🎀

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 25, 2022
@netlify
Copy link

netlify bot commented Feb 25, 2022

βœ”οΈ Yeeeehaw, deploy preview is ready!

πŸ”¨ Explore the source changes: b30eb44

πŸ” Inspect the deploy log: https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/6221654ce037ea0008906923

😎 Browse the preview: https://deploy-preview-1715--adoring-edison-dbcef8.netlify.app

Copy link
Contributor

@KemoPaw KemoPaw left a comment

Choose a reason for hiding this comment

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

Very cool to see the progress bar updating upon file uploads! πŸŽ‰ Sadly do not currently have a 4GB file to test for if a user can continue to upload past that point.

@josephmcg
Copy link
Contributor Author

I didn't either, I tested it by changing the limit to a smaller number

@stavares843 stavares843 added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Feb 28, 2022
@phillsatellite phillsatellite added QA tested One QA Person has tested - Needs QA Lead review still and removed Ready for QA Ready for QA team to test, Devs approved. labels Feb 28, 2022
@stavares843 stavares843 added the temporary blocked checking something QA Lead is checking something. label Feb 28, 2022
@phillsatellite phillsatellite removed the QA tested One QA Person has tested - Needs QA Lead review still label Feb 28, 2022
@phillsatellite
Copy link
Contributor

Tested: works really well, the only bug I was able to find is if a user has stuff uploaded and the refresh they're page, all the files will still be there but the progress bar will be empty until a user uploads another file

refresh.files.mov

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed temporary blocked checking something QA Lead is checking something. labels Feb 28, 2022
@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Feb 28, 2022
@stavares843
Copy link
Member

/rebase

@stavares843 stavares843 removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Feb 28, 2022
@josephmcg
Copy link
Contributor Author

@phillsatellite Thanks Phil! should be fixed now

@stavares843
Copy link
Member

thanks @josephmcg πŸ”¨

@stavares843 stavares843 added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 1, 2022
@josephmcg
Copy link
Contributor Author

@stavares843 I noticed something I need to fix here, will cause bugs after file type support gets merged

@josephmcg
Copy link
Contributor Author

ok, now its good

@stavares843
Copy link
Member

thanks πŸ’₯

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 1, 2022
@stavares843 stavares843 removed the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 1, 2022
@stavares843
Copy link
Member

/rebase

@josephmcg
Copy link
Contributor Author

josephmcg commented Mar 2, 2022

  • 4GB is 4e+9 bytes. A few bytes won't 'move the needle', so to speak. I'm not sure what the threshold is for a visible indication on <progress>. We could potentially add Math.ceil to it if you'd like to always see a bar, but that may have unintended results past 1%.
  • What should the threshold be for turning the text red? >80% filled?
  • I've never tried uploading several GB at once, I'll look into that

@stavares843
Copy link
Member

that's true!

maybe we could ask Liz about the 2 first questions as they are more UI/UX related

@josephmcg
Copy link
Contributor Author

@stavares843 issues above should be fixed. I was setting the loading state after checking filesize. It was working correctly in your video, it just takes a LONG time for files that large
It seems like the filesize package we're using changed their default byte base from 2 to 10. I added some config to change it to 2 again, now numbers should be more accurate

@josephmcg
Copy link
Contributor Author

Do we want to display in terms of decimal or binary? Before, we were using binary for calculations, but using the decimal labels.
https://en.wikipedia.org/wiki/Byte#Multiple-byte_units

@stavares843 stavares843 added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 2, 2022
@stavares843
Copy link
Member

thanks @josephmcg

we could ask if there's any preference, either decimal or binary

@molimauro molimauro added Ready for QA Ready for QA team to test, Devs approved. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Mar 3, 2022
@phillsatellite
Copy link
Contributor

@josephmcg hello!! Last little problem I found on this branch, after updates MB and GB changed to MiB and GiB
Screen Shot 2022-03-03 at 2 39 30 PM

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Mar 3, 2022
@josephmcg
Copy link
Contributor Author

yes, they were using binary. just switched it to decimal

@josephmcg josephmcg added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 4, 2022
@stavares843 stavares843 merged commit e68398e into dev Mar 4, 2022
@stavares843 stavares843 deleted the AP899 branch March 4, 2022 02:04
@github-actions github-actions bot removed the Ready for QA Ready for QA team to test, Devs approved. label Mar 4, 2022
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

5 participants