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

Add Download file action to file meatballs button #1011

Merged

Conversation

bergice
Copy link
Contributor

@bergice bergice commented Sep 25, 2019

No description provided.

client/src/containers/Editor/Editor.js Outdated Show resolved Hide resolved
client/src/containers/Editor/Editor.js Outdated Show resolved Hide resolved
client/src/containers/Editor/Editor.js Show resolved Hide resolved
code/Forms/FileFormFactory.php Outdated Show resolved Hide resolved
code/Forms/FileFormFactory.php Show resolved Hide resolved
client/src/containers/Editor/Editor.js Show resolved Hide resolved
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

I second everything @kinglozzer has said.

The "insert a link on the page and click on it" method feels kind of weird, but can't find a better way to trigger a download in JS.

Have you given some thoughts about trying to make the Download action an actual link? This would seem like the ideal approach. Would probably require more work, but it could be reuse in other places.

It would be nice to have some test coverage of this, but I'm not sure there's a nice elegant way of asserting that "my browser asked me to download this file". Seems like there's some way to do it in Behat but I haven't tried it.

@bergice bergice force-pushed the pulls/1/add-download branch from afd23e5 to 14995b0 Compare September 26, 2019 07:49
@bergice
Copy link
Contributor Author

bergice commented Sep 27, 2019

Have you given some thoughts about trying to make the Download action an actual link? This would seem like the ideal approach. Would probably require more work, but it could be reuse in other places.

It can be done with a LiteralField. See: vendor/silverstripe/asset-admin/code/Forms/FileFormFactory.php:103 - but it would require some CSS changes as well.

@maxime-rainville
Copy link
Contributor

Using a LiteralField might be a quick way to hack it in, but the correct way to do it probably involves updating something the FormBuilder in admin and updating it so that if a specific attribute on the FormAction gets specified, it renders it as a link instead of an actual button. If you feel like trying it out, we could have a look ... but it's very much a nice to have.

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Almost done. I still have a bit of an issue with the focus, but all my other concern have been addressed.

client/src/containers/Editor/Editor.js Show resolved Hide resolved
@maxime-rainville
Copy link
Contributor

I tested in Chrome, Firefox and IE11. It all works fine. Note that in IE11, it wants to display the images rather than download them. You might be able to force IE to download the files by adding a download attribute to the link, but I'm not sure.

@bergice
Copy link
Contributor Author

bergice commented Oct 8, 2019

I tested in Chrome, Firefox and IE11. It all works fine. Note that in IE11, it wants to display the images rather than download them. You might be able to force IE to download the files by adding a download attribute to the link, but I'm not sure.

I think this relies on browser settings and response headers in order to work.

@kinglozzer
Copy link
Member

Note that in IE11, it wants to display the images rather than download them. You might be able to force IE to download the files by adding a download attribute to the link, but I'm not sure.

Apparently IE doesn’t support the download attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#Browser_compatibility. We’d have to set up a new endpoint that adds the Content-Disposition header, so I think we should just leave it as a legacy browser quirk

@maxime-rainville maxime-rainville force-pushed the pulls/1/add-download branch 3 times, most recently from ccbb7a4 to 82070d6 Compare October 8, 2019 23:10
Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Everything works like a charm now. I've rebased this guy and sqashed all the commits.

Merge on green

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.

4 participants