Skip to content

Fix jump with font change#3529

Merged
swissspidy merged 9 commits intodevelopfrom
fix/3461-jump_with_font_change
Oct 17, 2019
Merged

Fix jump with font change#3529
swissspidy merged 9 commits intodevelopfrom
fix/3461-jump_with_font_change

Conversation

@miina
Copy link
Copy Markdown
Contributor

@miina miina commented Oct 15, 2019 •
edited
Loading

Summary

Fixes #3461
Fixes #3444

This PR creates a helper for getting an updated position after resizing. It also removes some seemingly redundant logic from resizing which seems to fix #3444

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label Oct 15, 2019
@miina
Copy link
Copy Markdown
Contributor Author

miina commented Oct 15, 2019

This PR needs thorough testing due to removing the logic of lastDelta* from resizing which seemed to have caused quite a few issues.

Important note: Since resizing for non-amp-fit-text Text blocks is broken right now due to #3454 then we should not merge this PR until that one is fixed and we can test properly!

@miina miina changed the title Fix jump with font change [WIP] Fix jump with font change Oct 15, 2019
@miina miina changed the title [WIP] Fix jump with font change Fix jump with font change Oct 16, 2019
barklund
barklund reviewed Oct 16, 2019
View reviewed changes
@barklund
Copy link
Copy Markdown
Contributor

barklund commented Oct 16, 2019

This looks promising! A great reduction of complexity by isolating a key calculation. I will have to look more into this to understand it fully, but it looks like a great effort!

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Oct 16, 2019

Hmm, just saw that some tests are failing, will look into that tomorrow.

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Oct 17, 2019

One thing I noticed that I was looking into #3444 .

The re-resizable component supports minHeight and minWidth. See the docs here. Min height and width are passed EnhancedResizableBox, however they are never passed ResizableBox as these fields are exacted from props so are not part of childProps. Not sure if it effects anything, but it just a strange.

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Oct 17, 2019

The reachedMinLimit is only used for min reached for text blocks is that correct. Why is this the case? Other types of blocks have min heights as well.

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Oct 17, 2019

The reachedMinLimit is only used for min reached for text blocks is that correct. Why is this the case? Other types of blocks have min heights as well.

reachedMinLimit is meant for tracking specifically if the resizing has reached the limit to be able to fit the text content inside:

amp-wp/assets/src/stories-editor/components/resizable-box/index.js

Lines 187 to 188 in 5799032

// Track if resizing has reached its minimum limits to fit the text inside.
let reachedMinLimit = false;

Ensuring that the minimum height and width for all the blocks would be respected happens here though:

amp-wp/assets/src/stories-editor/components/resizable-box/index.js

Lines 181 to 183 in 5799032

// If the new width/height is below the minimum limit, set the minimum limit as the width/height instead.
let appliedWidth = minWidth <= width + deltaW ? width + deltaW : minWidth;
let appliedHeight = minHeight <= height + deltaH ? height + deltaH : minHeight;

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Oct 17, 2019

I found the variable name reachedMinLimit a little confusing, as I thought it would be related to any block reaching it's min and not just text blocks. Might be worth changing the variable name for readability.

First of all, it would make the below lines

let appliedWidth = minWidth <= width + deltaW ? width + deltaW : minWidth;

more readable if they looked like this

let appliedWidth = Math.max ( minWidth, ( width + deltaW ) );

Why both with our own logic for min Height and width here?

@miina
Copy link
Copy Markdown
Contributor Author

miina commented Oct 17, 2019

Why both with our own logic for min Height and width here?

We need to determine the appliedWidth and appliedHeight for being able to do the correct position calculations, it's not only for stopping resizing to respect the limits.

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Oct 17, 2019

Other than an alignment, the code seems good. Once that change was made, I will approve.

spacedmonkey
spacedmonkey approved these changes Oct 17, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@swissspidy swissspidy added this to the v1.4 milestone Oct 17, 2019
@swissspidy swissspidy merged commit d246e01 into develop Oct 17, 2019
@swissspidy swissspidy deleted the fix/3461-jump_with_font_change branch October 17, 2019 16:34
@miina miina mentioned this pull request Oct 17, 2019
Closed
westonruter added a commit that referenced this pull request Oct 18, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into improve/v…
1c89197 
…alidation-sourcing

* 'develop' of github.com:ampproject/amp-wp: (57 commits)
  Undo over-eager rename of auto_accept_sanitization
  Rename auto-accept to accept-by-default
  Rename filter to amp_validation_error_auto_sanitized
  Fix jumping blocks when changing fonts (#3529)
  Update dependency browserslist to v4.7.1 (#3551)
  Make sure that isPageBlock always returns boolean.
  Fix and improve e2e tests.
  Remove unnessary check for block. Use shift over access first element of array.
  Use isPageBlock for cleaner code.
  Fix minor grammar issues
  Add docblock for `amp_is_sanitization_auto_accepted` filter
  Align arrows
  Update tests to use filter to set sanitization auto acceptance
  Make AMP_Validation_Manage::is_sanitization_auto_accepted() filterable
  Remove 'auto_accept_sanitization' option
  Remove relevant auto-sanitization notices
  Remove validation handling setting field
  Add special-case handling for layout=fill when both dimensions are 100%
  Insert duplicated page as current page.
  Ensure smooth snap line display (#3514)
  ...
westonruter added a commit that referenced this pull request Oct 18, 2019
@westonruter
Merge branch 'develop' of github.com:ampproject/amp-wp into add/3342-…
ab98bab 
…twentytwenty-support

* 'develop' of github.com:ampproject/amp-wp: (73 commits)
  Mock HTTP request for Test_AMP_WordPress_TV_Embed_Handler
  Add test case
  Fix issues with image display (#3555)
  Fix infinite loop when uploading videos (#3553)
  Fix styling on remove button. (#3554)
  Test for XML processing instruction
  Undo over-eager rename of auto_accept_sanitization
  Rename auto-accept to accept-by-default
  Rename filter to amp_validation_error_auto_sanitized
  Fix jumping blocks when changing fonts (#3529)
  Update dependency browserslist to v4.7.1 (#3551)
  Make sure that isPageBlock always returns boolean.
  Fix and improve e2e tests.
  Remove unnessary check for block. Use shift over access first element of array.
  Use isPageBlock for cleaner code.
  Fix minor grammar issues
  Add docblock for `amp_is_sanitization_auto_accepted` filter
  Align arrows
  Update tests to use filter to set sanitization auto acceptance
  Make AMP_Validation_Manage::is_sanitization_auto_accepted() filterable
  ...
@spacedmonkey spacedmonkey mentioned this pull request Oct 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy Awaiting requested review from swissspidy

2 more reviewers

@barklund barklund barklund left review comments

@spacedmonkey spacedmonkey spacedmonkey 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.4

Development

Successfully merging this pull request may close these issues.

Text block jumps when changing normal to huge font Blocks "jumping" when resizing to minimum limits fast

5 participants

@miina @barklund @spacedmonkey @swissspidy @googlebot