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

Refactor File block to use block.json metadata #14862

Merged
merged 2 commits into from
May 6, 2019
Merged

Refactor File block to use block.json metadata #14862

merged 2 commits into from
May 6, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 8, 2019 •
edited
Loading

Description

Partially implements #13693:

  • Refactors File block to define name, category and attributes in block.json
  • Moves all fields defined on the client like transforms or save to their own files

How has this been tested?

Ensure that File block still works as before.

@gziolo gziolo added [Type] Task Issues or PRs that have been broken down into an individual action to take [Block] File Affects the File Block [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels Apr 8, 2019
@gziolo gziolo self-assigned this Apr 8, 2019
@youknowriad youknowriad mentioned this pull request Apr 8, 2019
Merged
gziolo
gziolo commented Apr 8, 2019
View reviewed changes
packages/block-library/src/file/index.js
type: 'string',
source: 'html',
selector: 'a[download]',
default: _x( 'Download', 'button label' ),
Copy link
Member Author

Choose a reason for hiding this comment

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

It was discussed on WordPress.org Slack last week (link requires registration):

https://wordpress.slack.com/archives/C5UNMSU4R/p1554217879115800

The conclusion was that:

This doesn't really sound like a "default" but an attribute set when the block is first inserted.

To make it work with block.json I converted it to the static text. In the previous form, it was confusing enough. It wasn't triggering block validation when editing the same post with a different locale selected, however, it's worth pointing out that the locale of the website may differ from the one that the person editing uses. It's better to make it static and let the author update this value.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the decision here was to set this initially to the value of the author's language. In other words, we could use componentDidMount or useEffect in the edit function of the block to set its value when the block is first created. This has the downside of creating an "undo" level maybe but maybe that's fine?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll give it a go and see how it works 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

This has the downside of creating an "undo" level maybe but maybe that's fine?

It is precisely what happens. Is there a way to skip adding undo level when calling setAttributes? I noticed that this is an issue also when using drag & drop with files. Triggering undo puts the block in a bizarre pulsing state in UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

file-block-undo

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth and @ellatrix any thoughts on this undo behavior for the transient state of the block attributes? Do you know if there is any issue or PR opened which tries to solve it?

Copy link
Member

Choose a reason for hiding this comment

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

@aduth and @ellatrix any thoughts on this undo behavior for the transient state of the block attributes? Do you know if there is any issue or PR opened which tries to solve it?

#8119 is the main issue tracking the problems in this area.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, thanks for linking with the issue. It looks like it isn’t a blocker for this PR 👍

@gziolo gziolo mentioned this pull request Apr 8, 2019
Merged
@gziolo gziolo force-pushed the update/file-block-json branch 4 times, most recently from 16a92f9 to dd981c3 Compare April 14, 2019 13:32
@gziolo gziolo requested a review from a team April 19, 2019 12:20
youknowriad
youknowriad approved these changes May 6, 2019
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Seems to work as intended.

@gziolo gziolo merged commit be8add9 into master May 6, 2019
@gziolo gziolo deleted the update/file-block-json branch May 6, 2019 14:01
@gziolo gziolo added this to the 5.7 (Gutenberg) milestone May 6, 2019
@gziolo gziolo mentioned this pull request Jun 18, 2019
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth left review comments

@youknowriad youknowriad youknowriad approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@noisysocks noisysocks Awaiting requested review from noisysocks

@notnownikki notnownikki Awaiting requested review from notnownikki

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

Assignees

@gziolo gziolo

Labels
[Block] File Affects the File Block [Feature] Block Directory Related to the Block Directory, a repository of block plugins [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Milestone
5.7 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
3 participants
@gziolo @youknowriad @aduth