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

Fix: File Block is crashing as soon as a file is selected #10323

Merged
merged 2 commits into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Oct 3, 2018

Description

The file block was not updated to use the new RichText content structure and its old implementation had inconsistencies (string in the attributes, and not a string in one of the RichText).
So the file block was crashing as soon as a file was selected.
This PR updates file block to use the new RichText format.

Props to @iseulde for the help debugging this problem.

How has this been tested

Add a file block select some file, verify it is possible to change both file name and button text without problems. Save the post and verify the block appears correctly on the frontend.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Oct 3, 2018

Member

You could argue that the deprecations are not working here, but it's important to note that the wrong format prop is being set on RichText. Additionally the multiline prop is supposed to be a tag name, not a boolean. So this fix would also work, but it's good to update the format. Not sure if we should maybe account for cases where plugins might pass multiline="false", but it's just using it completely wrong. Cc @mcsf.

diff --git a/packages/block-library/src/file/edit.js b/packages/block-library/src/file/edit.js
index 49b57c922..310468dea 100644
--- a/packages/block-library/src/file/edit.js
+++ b/packages/block-library/src/file/edit.js
@@ -191,7 +191,6 @@ class FileEdit extends Component {
                                                        tagName="div" // must be block-level or else cursor disappears
                                                        format="string"
                                                        value={ fileName }
-                                                       multiline="false"
                                                        placeholder={ __( 'Write file name…' ) }
                                                        keepPlaceholderOnFocus
                                                        formattingControls={ [] } // disable controls
@@ -202,12 +201,12 @@ class FileEdit extends Component {
                                                                { /* Using RichText here instead of PlainText so that it can be styled like a button */ }
                                                                <RichText
                                                                        tagName="div" // must be block-level or else cursor disappears
+                                                                       format="string"
                                                                        className={ `${ className }__button` }
                                                                        value={ downloadButtonText }
                                                                        formattingControls={ [] } // disable controls
                                                                        placeholder={ __( 'Add text…' ) }
                                                                        keepPlaceholderOnFocus
-                                                                       multiline="false"
                                                                        onChange={ ( text ) => setAttributes( { downloadButtonText: text } ) }
                                                                />
                                                        </div>
Member

iseulde commented Oct 3, 2018

You could argue that the deprecations are not working here, but it's important to note that the wrong format prop is being set on RichText. Additionally the multiline prop is supposed to be a tag name, not a boolean. So this fix would also work, but it's good to update the format. Not sure if we should maybe account for cases where plugins might pass multiline="false", but it's just using it completely wrong. Cc @mcsf.

diff --git a/packages/block-library/src/file/edit.js b/packages/block-library/src/file/edit.js
index 49b57c922..310468dea 100644
--- a/packages/block-library/src/file/edit.js
+++ b/packages/block-library/src/file/edit.js
@@ -191,7 +191,6 @@ class FileEdit extends Component {
                                                        tagName="div" // must be block-level or else cursor disappears
                                                        format="string"
                                                        value={ fileName }
-                                                       multiline="false"
                                                        placeholder={ __( 'Write file name…' ) }
                                                        keepPlaceholderOnFocus
                                                        formattingControls={ [] } // disable controls
@@ -202,12 +201,12 @@ class FileEdit extends Component {
                                                                { /* Using RichText here instead of PlainText so that it can be styled like a button */ }
                                                                <RichText
                                                                        tagName="div" // must be block-level or else cursor disappears
+                                                                       format="string"
                                                                        className={ `${ className }__button` }
                                                                        value={ downloadButtonText }
                                                                        formattingControls={ [] } // disable controls
                                                                        placeholder={ __( 'Add text…' ) }
                                                                        keepPlaceholderOnFocus
-                                                                       multiline="false"
                                                                        onChange={ ( text ) => setAttributes( { downloadButtonText: text } ) }
                                                                />
                                                        </div>

@jorgefilipecosta jorgefilipecosta added this to the 4.0 milestone Oct 3, 2018

@iseulde

iseulde approved these changes Oct 4, 2018

Looks good!

@jorgefilipecosta jorgefilipecosta merged commit 4a47dff into master Oct 4, 2018

2 checks passed

codecov/project 49.43% (+<.01%) compared to 4691baf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/file-block-bug branch Oct 4, 2018

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Oct 4, 2018

Member

Thank you for the review and for the help in getting this PR done @iseulde.

Member

jorgefilipecosta commented Oct 4, 2018

Thank you for the review and for the help in getting this PR done @iseulde.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Oct 4, 2018

Contributor

Thanks for the fixes to both.

  1. We have a few places, including tests, where booleans are passed as multiline, so it seems that we should log a warning, whether phrased as a deprecation or not.
  2. It's not great if blocks from the core library show these issues, so can we prevent failure there? I can imagine issues with third-party blocks that follow our block implementations.

@jorgefilipecosta @iseulde

Contributor

mcsf commented Oct 4, 2018

Thanks for the fixes to both.

  1. We have a few places, including tests, where booleans are passed as multiline, so it seems that we should log a warning, whether phrased as a deprecation or not.
  2. It's not great if blocks from the core library show these issues, so can we prevent failure there? I can imagine issues with third-party blocks that follow our block implementations.

@jorgefilipecosta @iseulde

@iseulde iseulde referenced this pull request Oct 16, 2018

Merged

RichText: Ensure multiline prop is either "p" or "li" #10664

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment