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' attr in anchor of file block #9346

Closed
wants to merge 1 commit into from
Closed

Add 'download' attr in anchor of file block #9346

wants to merge 1 commit into from

Conversation

barcia
Copy link

@barcia barcia commented Aug 25, 2018

Description

Regardless of the download button is shown or not, the link is also intended to perform the download, so the anchor must also have the 'download' attribute

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • [x ] My code follows the WordPress code style.
  • [ x] My code follows the accessibility standards.
  • [ x] My code has proper inline documentation.

Regardless of the download button is shown or not, the link is also intended to perform the download, so the anchor must also have the 'download' attribute
// ensure download attribute is still set when fileName
// is undefined. Using '' here as `true` still leaves
// the attribute unset.
download={ fileName || '' }
Copy link
Contributor

Choose a reason for hiding this comment

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

This make sense to me. Though changing the HTML output of any block means we need to provide a "deprecated version" to ensure old blocks saved with the old markup are "upgraded" when we open the post in the editor.

See https://wordpress.org/gutenberg/handbook/block-api/deprecated-blocks/

Also, make sure to check the unit tests, there's probably a fixture to update.

Copy link
Author

Choose a reason for hiding this comment

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

I understand, however, I am web designer, and I'm not the knowledge to do this… I read the documentation but I don't know where to start.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, I'll start to help when I have some time. The idea is that you'll have to copy the previous "save" function and "attributes" definition into a "deprecated version"

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I noticed this change causes an error where the block become invalid after saving it and refreshing the page. Maybe there's a good reason for omitting the download attribute here (it's present in the download button). So I'm pinging @noisysocks for more info.

@markolbert
Copy link

I hope support for the html5 download attribute gets added. Right now if you add "download" as an attribute in the code editor, it gets changed to download="" upon save, which keeps the download attribute from being properly interpreted, at least in the latest chrome browser.

@gziolo gziolo added the [Block] File Affects the File Block label Feb 1, 2019
@gziolo gziolo requested review from noisysocks and talldan February 7, 2019 16:28
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Feb 7, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@talldan
Copy link
Contributor

talldan commented Feb 8, 2019

The PR lacks a related issue so I'm not completely sure about the the motivation, would be good to understand more about that. Also worth referencing the original issue for the file block - #5462. It didn't have a great deal of discussion, but it is mentioned a few times about the link being an actual link. The user can also still download the file by right-clicking if needed.

Worth noting this other related PR (though it was only merged after this one was created), we only support download attributes that are true or undefined:
#10976

@gziolo gziolo added the [Status] Needs More Info Follow-up required in order to be actionable. label Mar 8, 2019
@gziolo gziolo removed this from the 5.3 (Gutenberg) milestone Mar 8, 2019
@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

This PR seems to be abandoned. Let's close it as we can't really proceed without having more details from the author. @barcia thank you for your contribution. Feel free to reopen this PR once you get back to it and answer to the @talldan's comment.

@gziolo gziolo closed this Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] File Affects the File Block [Status] Needs More Info Follow-up required in order to be actionable. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants