Skip to content

Fix resizing for formatting blocks#2562

Merged
swissspidy merged 12 commits intodevelopfrom
fix/2553-resizing
Jun 11, 2019
Merged

Fix resizing for formatting blocks#2562
swissspidy merged 12 commits intodevelopfrom
fix/2553-resizing

Conversation

@miina
Copy link
Copy Markdown
Contributor

@miina miina commented Jun 11, 2019

Fixes #2553.

Does the following:

  • Verse and Preformatted blocks: Removes resizing, based on how it displays in the frontend and the purpose of the blocks, it might not really make sense to resize these blocks -- it won't have an effect in the FE anyway. It could make sense if these blocks would have background color, we could try to implement it together with that later.
  • Table block: adjusts the minimum allowed height for resizing based on rows of the table.
  • Code block: Restricts resizing the height smaller than the text allows. Also hides the overflow to match the FE and editor.

@googlebot googlebot added the cla: yes Signed the Google CLA label Jun 11, 2019
@miina miina requested a review from swissspidy June 11, 2019 11:21
swissspidy
swissspidy reviewed Jun 11, 2019
View reviewed changes
assets/src/stories-editor/constants.js

export const MIN_BLOCK_WIDTH = 40;
export const MIN_BLOCK_HEIGHT = 30;
export const MIN_BLOCK_HEIGHTS = {
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy Jun 11, 2019

Choose a reason for hiding this comment

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

Can we somehow consolidate this with getDefaultMinimumBlockHeight?

Copy link
Copy Markdown
Contributor Author

@miina miina Jun 11, 2019

Choose a reason for hiding this comment

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

Hmm, I'm not sure because one of these is used for the initial default height and the other one is for the minimum allowed resizing. The minimum allowed heights don't necessarily make sense as the default heights. Thoughts?

Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy Jun 11, 2019

Choose a reason for hiding this comment

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

Fair enough. A mention in the docs would be helpful though.

swissspidy
swissspidy reviewed Jun 11, 2019
View reviewed changes
assets/src/stories-editor/components/higher-order/with-amp-story-settings.js
blockElementId={ `block-${ clientId }` }
initialAngle={ rotationAngle }
className="amp-story-editor__rotate-container"
angle={ rotationAngle }
Copy link
Copy Markdown
Collaborator

@swissspidy swissspidy Jun 11, 2019

Choose a reason for hiding this comment

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

Looks like this causes prop type invalidation when rotationAngle is undefined. I guess we need to make the prop optional in the prop types definition.

Will have a look.

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Jun 11, 2019

@miina The test for the amp/amp-story-cta is currently failing because apparently BlockEdit is rendered twice in withAmpStorySettings. You can easily verify this:

  1. Create new story
  2. Create a second page
  3. Insert CTA block
  4. Notice that the edit component is rendered twice.

The ( ! isMovableBlock || isEmptyImageBlock ) check and the ! needsResizing checks are both true for the CTA block.

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Jun 11, 2019

BlockEdit is rendered twice in withAmpStorySettings

Thanks! Added isMovableBlock condition within 03bd647.

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Jun 11, 2019

@miina Does the table block also look like this for you?

Screenshot 2019-06-11 at 17 19 26

@swissspidy swissspidy requested a review from westonruter June 11, 2019 16:24
@miina
Copy link
Copy Markdown
Contributor Author

miina commented Jun 11, 2019

Hmm, it doesn't look exactly like this but it's definitely off, the styling. Checking shortly

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Jun 11, 2019

@miina Does the table block also look like this for you?

Should look better now.

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Jun 11, 2019

@kienstra If you have time, would you mind testing this PR a bit as well?

@kienstra
Copy link
Copy Markdown
Contributor

kienstra commented Jun 11, 2019

Sure, @miina! I'll test it now.

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Jun 11, 2019

There's a merge conflict that should be resolved prior to testing.

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Jun 11, 2019

Fixed conflicts now

@swissspidy swissspidy requested a review from kienstra June 11, 2019 17:48
kienstra
kienstra approved these changes Jun 11, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

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

Approved

Hi @miina,
This looks good, and works as described.

resizing-works-well

I also saw that resizing isn't allowed for the Verse and Preformatted blocks.

And there's a good minimum height for the Table and Code blocks:

code-resizing

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Jun 11, 2019

Thank you for testing, @kienstra.

@swissspidy swissspidy merged commit f6f1162 into develop Jun 11, 2019
@swissspidy swissspidy deleted the fix/2553-resizing branch June 11, 2019 19:37
@westonruter westonruter added this to the v1.2 milestone Jun 11, 2019
westonruter added a commit that referenced this pull request Jun 11, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into update/gu…
5040a40 
…ide-lighter-media

* 'develop' of github.com:ampproject/amp-wp:
  Fix resizing for formatting blocks (#2562)
  Prevent block invalidation due to missing width/height (#2557)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy swissspidy left review comments

@westonruter westonruter Awaiting requested review from westonruter

+1 more reviewer

@kienstra kienstra kienstra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes Signed the Google CLA

Projects

None yet

Milestone

v1.2

Development

Successfully merging this pull request may close these issues.

Resizing going smaller than the block for less common blocks

5 participants

@miina @swissspidy @kienstra @westonruter @googlebot