Skip to content

Fix resize for non-fit text box#3259

Merged
swissspidy merged 3 commits intodevelopfrom
fix/3199-resize-textbox
Sep 18, 2019
Merged

Fix resize for non-fit text box#3259
swissspidy merged 3 commits intodevelopfrom
fix/3199-resize-textbox

Conversation

@barklund
Copy link
Copy Markdown
Contributor

@barklund barklund commented Sep 16, 2019

Fixes #3199.

Reducing the size of non-fit text box was broken recently - possibly due changes upstream. The fixed height of text box is required for other section of the code to work correctly, however it broke reducing size here.

By setting the text box height to auto before calculating scrollHeight, the true height can once again be established - original height will be reset after change.

Furthermore, this commit also changes the <= to < in the limiting condition - to allow one-directional resize.

However there are still weird issues for rotated text boxes when hitting the minimum size, but that have probably existed for some time.

Please note, if you are reviewing this, play around with both fitted and non-fitted text boxes as well as other types of elements and try to resize them both larger and smaller in all directions. Furthermore, do this when rotated as well.

@barklund
Fix resize for nonfit textbox
7ed5258 
Reducing the size of nonfit textbox was broken - possibly due changes upstream. The fixed height of textbox is required for other section of the code to work correctly, however it broke reducing size here.

By setting the textbox height to `auto` before calculating `scrollHeight`, the true height can once again be established - original height willl be reset after change.

Furthermore, this commit also changes the `<=` to `<` in the limiting condition - to allow one-directional resize.

However there are still weird issues for rotated textboxes when hitting the minimum size, but that have probably existed for some time.
@barklund barklund added Bug Something isn't working AMP Stories labels Sep 16, 2019
@barklund barklund added this to the v1.3 milestone Sep 16, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 16, 2019
@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Sep 17, 2019

This fixes #3199 for me at first glance. Will test it a bit more though.

However there are still weird issues for rotated text boxes when hitting the minimum size, but that have probably existed for some time.

Do you mean this? https://cloudup.com/cXICc_z3pah

@barklund
Copy link
Copy Markdown
Contributor Author

barklund commented Sep 17, 2019

Do you mean this? https://cloudup.com/cXICc_z3pah

Exactly that. Equally, but differently, weird at all angles.

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Sep 17, 2019

This might be related, WordPress/gutenberg#17011 . Upstream package was updated. This might have effected something here.

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Sep 18, 2019

Screenshot from 2019-09-18 10-07-54
I can still see this weird sizing when I paste text into the editor.

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Sep 18, 2019

I can still see this weird sizing when I paste text into the editor.

Hmm this should be caught in the text block's edit component's componentDidUpdate method:

amp-wp/assets/src/stories-editor/blocks/amp-story-text/edit.js

Lines 70 to 79 in 93276f7

const checkBlockDimensions = ! ampFitText && (
! isEqual( prevProps.fontSize, fontSize ) ||
prevProps.attributes.ampFitText !== ampFitText ||
prevProps.attributes.ampFontFamily !== ampFontFamily ||
prevProps.attributes.content !== content
);
if ( checkBlockDimensions ) {
maybeUpdateBlockDimensions( this.props );
}

amp-wp/assets/src/stories-editor/helpers/index.js

Lines 1262 to 1315 in 93276f7

/**
* Updates a block's width and height in case it doesn't use amp-fit-text and the font size has changed.
*
* @param {Object} block Block object.
* @param {string} block.clientId Block client ID.
* @param {Object} block.attributes Block attributes.
* @param {number} block.attributes.width Block width in pixels.
* @param {number} block.attributes.height Block height in pixels.
* @param {string} block.attributes.content Block inner content.
* @param {boolean} block.attributes.ampFitText Whether amp-fit-text should be used or not.
* @param {number} block.attributes.autoFontSize Automatically determined font size for amp-fit-text blocks.
*/
export const maybeUpdateBlockDimensions = ( block ) => {
const { name, clientId, attributes } = block;
const { width, height, ampFitText, content } = attributes;
if ( ampFitText ) {
return;
}
switch ( name ) {
case 'amp/amp-story-text':
const element = getBlockInnerTextElement( block );
if ( element && content.length ) {
if ( element.offsetHeight > height ) {
updateBlockAttributes( clientId, { height: element.offsetHeight } );
}
if ( element.offsetWidth > width ) {
updateBlockAttributes( clientId, { width: element.offsetWidth } );
}
}
break;
case 'amp/amp-story-post-title':
case 'amp/amp-story-post-author':
case 'amp/amp-story-post-date':
const metaBlockElement = getBlockInnerTextElement( block );
if ( metaBlockElement ) {
metaBlockElement.classList.toggle( 'is-measuring' );
if ( metaBlockElement.offsetHeight > height ) {
updateBlockAttributes( clientId, { height: metaBlockElement.offsetHeight } );
}
if ( metaBlockElement.offsetWidth > width ) {
updateBlockAttributes( clientId, { width: metaBlockElement.offsetWidth } );
}
metaBlockElement.classList.toggle( 'is-measuring' );
}

Perhaps it needs the same height fix there so the element.offsetHeight is correct.

@spacedmonkey
Copy link
Copy Markdown
Contributor

spacedmonkey commented Sep 18, 2019

@swissspidy Should this be part of this PR or another one? Is it related?

@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Sep 18, 2019

Semi-related, but can be looked at in a separate PR.

This was referenced Sep 18, 2019
Closed
Closed
@swissspidy
Copy link
Copy Markdown
Collaborator

swissspidy commented Sep 18, 2019

Created two new issues for these bugs: #3290 and #3291.

@swissspidy swissspidy merged commit bb8dd4b into develop Sep 18, 2019
@swissspidy swissspidy deleted the fix/3199-resize-textbox branch September 18, 2019 10:57
@swissspidy swissspidy mentioned this pull request Sep 23, 2019
Closed
@spacedmonkey spacedmonkey mentioned this pull request Oct 1, 2019
Closed
westonruter added a commit that referenced this pull request Oct 1, 2019
@westonruter
Reset to tag '1.3.0'
2c66bfe 
* tag '1.3.0': (318 commits)
  Bump 1.3.0
  Add inline styles for custom fonts (#3345)
  Limit deeply-nesting test to 200 to fix Xdebug error (#3341)
  Bump 1.3-RC2 (#3335)
  Sanitize invalid children of amp-story and amp-story-page elements to prevent white story of death (#3336)
  Remove unused Travis deploy stage (#3340)
  Implement automated accessibility testing using Axe (#3294)
  Only add all Google Font style rules in editor context
  Prevent adding AMP query var to Story URLs in Compatibility Tool
  Prevent attempting to redirect Stories with rejected validation errors
  Ensure all AMP scripts (including v0.js) get moved to the head
  Make sure that media picker is background types are filter correctly.
  Normalize style[type] attribute quote style after r46164 in WP core
  Fix phpunit covers tags
  Bump version to 1.3-RC1
  Strip 100% width/height from layout=fill elements
  Fix issue with cut (#3246)
  Remove unused Google Fonts SVGs (#3289)
  Fix resize for non-fit text box (#3259)
  Use template_dir consistently as signal for transitional mode
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@swissspidy swissspidy swissspidy approved these changes

@spacedmonkey spacedmonkey Awaiting requested review from spacedmonkey

@dero dero Awaiting requested review from dero

Assignees

No one assigned

Labels

Bug Something isn't working cla: yes Signed the Google CLA

Projects

None yet

Milestone

v1.3

Development

Successfully merging this pull request may close these issues.

Non-fitted text blocks cannot be resized smaller

4 participants

@barklund @swissspidy @spacedmonkey @googlebot