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.

Sign up for GitHub

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

Jump to bottom

Storage: Download functionality on bucket files #1424

Merged
merged 12 commits into from
Aug 11, 2021
Merged

Storage: Download functionality on bucket files #1424

merged 12 commits into from
Aug 11, 2021

Conversation

RyRy79261
Copy link
Contributor

closes #1418

@RyRy79261 RyRy79261 added the Status: Review Needed 👀 Added to PRs when they need more review label Aug 6, 2021
@RyRy79261 RyRy79261 self-assigned this Aug 6, 2021
@render
Copy link

render bot commented Aug 6, 2021

Your Render PR Server URL is https://files-ui-stage-pr-1424.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c46gfvsobjd906i9hg40.

@render
Copy link

render bot commented Aug 6, 2021

Your Render PR Server URL is https://storage-ui-stage-pr-1424.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c46gg0cobjd906i9hgkg.

@render
Copy link

render bot commented Aug 6, 2021

Your Render PR Server URL is https://gaming-ui-stage-pr-1424.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c46gg1cobjd906i9hh10.

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Aug 6, 2021
Tbaut
Tbaut reviewed Aug 6, 2021
View reviewed changes
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Hmm looks like the discussion we had with Mike on the issue was ignored. Overall I'm not willing to fight for it, just I think it'd have been nice if any consensus achieved on the development would be followed.

packages/storage-ui/src/Components/Pages/BucketPage.tsx Outdated
throw new Error("Not implemented")
// const itemToDownload = pathContents.find(item => item.cid === cid)
// if (!itemToDownload || !bucket) return
// throw new Error("Not implemented")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// throw new Error("Not implemented")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tbaut What was discussed regarding downloads? The ticket seemed straighforward

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check the comments, the description was subsequently updated. #1418

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I should hide the progress bar?

Copy link
Collaborator

@Tbaut Tbaut Aug 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nope, the idea was to not handle the download in js, but rather download the file using a gateway directly. So get the cid, triggers a browser download (with a <a download= or something similar).

Copy link
Collaborator

Choose a reason for hiding this comment

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

But again, I'm not willing to fight for this. Feel free to re-open, I'd approve the PR.

packages/storage-ui/src/Contexts/StorageContext.tsx Outdated Show resolved Hide resolved
RyRy79261 and others added 3 commits August 6, 2021 13:08
@RyRy79261 @Tbaut
Update packages/storage-ui/src/Components/Pages/BucketPage.tsx
8dfea1f 
Co-authored-by: Thibaut Sardan <33178835+Tbaut@users.noreply.github.com>
@RyRy79261 @FSM1
Update packages/storage-ui/src/Components/Pages/BucketPage.tsx
22aaa75 
Co-authored-by: Michael Yankelev <12774278+FSM1@users.noreply.github.com>
@Tbaut
Merge branch 'dev' into feat/download-buckets-1418
9169ba1
@RyRy79261 RyRy79261 closed this Aug 6, 2021
@FSM1 FSM1 reopened this Aug 6, 2021
@render
Copy link

render bot commented Aug 6, 2021

Your Render PR Server URL is https://storage-ui-stage-pr-1424.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c46o7mcobjd906i9psbg.

@render
Copy link

render bot commented Aug 6, 2021

Your Render PR Server URL is https://gaming-ui-stage-pr-1424.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c46o7msobjd906i9psog.

@render
Copy link

render bot commented Aug 6, 2021

Your Render PR Server URL is https://files-ui-stage-pr-1424.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-c46o7ncobjd906i9pt40.

@asnaith
Copy link
Member

asnaith commented Aug 7, 2021

The download functionality is working well.

I think the CSS changes have caused this issue though:

Screen Shot 2021-08-06 at 8 16 14 PM

@Tbaut
Copy link
Collaborator

Tbaut commented Aug 9, 2021

Just the comment from Andrew, otherwise looks good

@RyRy79261
Copy link
Contributor Author

@asnaith When I started the ticket that column wasn't present, I mentioned to @FSM1 but I think he made another ticket for it, need to confirm

@FSM1
Copy link
Contributor

FSM1 commented Aug 10, 2021

@asnaith When I started the ticket that column wasn't present, I mentioned to @FSM1 but I think he made another ticket for it, need to confirm

Issue is now created here: #1431

@Tbaut
Copy link
Collaborator

Tbaut commented Aug 10, 2021 •
edited
Loading

From what I can see @RyRy79261 this problem was introduced here, I reverted your changes in 92ce96f
Is there any reason you removed the import in the first place? These are here so that we set the column width in one place for the headers and all the rows, let me know if we should fix it differently.

edit: looks good now from my tests at least.

@Tbaut Tbaut mentioned this pull request Aug 10, 2021
Closed
@FSM1 FSM1 merged commit 31b6739 into dev Aug 11, 2021
@FSM1 FSM1 deleted the feat/download-buckets-1418 branch August 11, 2021 12:55
@RyRy79261
Copy link
Contributor Author

I wasn't aware we had abstracted that yet so might of missed the change during a merge

@RyRy79261
Copy link
Contributor Author

@Tbaut I branched off dev and I'm seeing this issue now:
image

@RyRy79261 RyRy79261 mentioned this pull request Aug 11, 2021
Merged
@Tbaut
Copy link
Collaborator

Tbaut commented Aug 12, 2021 •
edited
Loading

Ah small misunderstanding I think. The issue that you see is the fact that it's taking all the space, and shrinking the name right? This is something I saw, but didn't consider as a problem. What Andrew reported #1424 (comment) was the cid overflowing on the size, which is what I intended to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FSM1 FSM1 FSM1 approved these changes

@Tbaut Tbaut Tbaut approved these changes

@asnaith asnaith Awaiting requested review from asnaith

@tanmoyAtb tanmoyAtb Awaiting requested review from tanmoyAtb

Assignees

@RyRy79261 RyRy79261

Labels
Status: Review Needed 👀 Added to PRs when they need more review Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow for downloading files from a bucket
4 participants
@RyRy79261 @asnaith @Tbaut @FSM1