Fix/column width units #26757
Fix/column width units #26757
Conversation
tellthemachines
requested review from
ajitbohra,
Soean and
talldan
as code owners
tellthemachines
added
[Block] Columns
|
Size Change: +47 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hey @tellthemachines - thanks for working on this. I tested it and seems fine! I'm not really sure about the deprecation though about what cases wants to handle..
| Number.isFinite( block.attributes.width ) | ||
| ); | ||
| export function hasExplicitPercentColumnWidths( blocks ) { | ||
| return blocks.every( ( block ) => { |
There was a problem hiding this comment.
I think you can simplify this a bit like:
const blockWidth = block.attributes.width;
return Number.isFinite(
blockWidth.endsWith?.( '%' ) ? parseFloat( blockWidth ) : blockWidth
);
There was a problem hiding this comment.
Ooooh, much better, thanks!
|
|
||
| const deprecated = [ | ||
| { |
There was a problem hiding this comment.
I'm not really sure about the deprecation here. For start it should have attributes or isEligible etc.. With just a save function, wouldn't it match if a Columns block was invalid for whatever reason? -cc @gziolo
There was a problem hiding this comment.
It would work without attributes, but as soon as they change, it might stop working. So it's safer to set them. isEligible is optional but it might be good idea if only width is important to trigger this deprecation.
|
|
||
| const deprecated = [ | ||
| { |
There was a problem hiding this comment.
It would work without attributes, but as soon as they change, it might stop working. So it's safer to set them. isEligible is optional but it might be good idea if only width is important to trigger this deprecation.
| } ); | ||
|
|
||
| let style; | ||
| if ( width ) { |
There was a problem hiding this comment.
Is it possible to modify the previous deprecation instead? The one that created the issue in the first place?
There was a problem hiding this comment.
If we have https://github.com/WordPress/gutenberg/pull/26757/files#r518737391 + #26774, then we might not need another deprecation.
|
It would be great to add some tests that verify if the originally introduced deprecation work as expected in the first place. We should also add a block fixture that is buggy to ensure it's resolved. |
|
I opened #26774 that adds tests for the deprecation introduced in #24711. It also improves a bit the setup for the deprecation based on the suggestions from @ntsekouras. It would be nice to land first and rebase this PR to ensure this test still passes. |
| @@ -16,7 +16,10 @@ export default function save( { attributes } ) { | |||
| } ); | |||
|
|
|||
| let style; | |||
| if ( width ) { | |||
|
|
|||
| if ( Number.isFinite( width ) ) { | |||
There was a problem hiding this comment.
width can be undefined, so we still should keep the check, how about (I included code comment to ensure it doesn't get removed:
if ( width ) {
// Numbers are handled for backward compatibility as they can be still provided with templates.
style = { flexBasis: Number.isFinite( width ) ? width + '%' : width };
}
tellthemachines
force-pushed
the
fix/column-width-units
branch
from
a234102 to
bf3fe09
Compare
|
I addressed all the feedback, removed the deprecation and ran the integration tests locally; everything looks fine. Also tested with a few column blocks created on a previous branch and nothing breaks in the editor. E2e failures don't seem related to this work; they're failing on every PR now 😩 Would be good to get this merged today though, so we can include it in Beta 4. |
| if ( width ) { | ||
| style = { flexBasis: width }; | ||
| // Numbers are handled for backward compatibility as they can be still provided with templates. | ||
| style = { flexBasis: Number.isFinite( width ) ? width + '%' : width }; |
There was a problem hiding this comment.
I will explore it further, but seeing this PR again, I started thinking about whether new changes applied to edit are enough. Anyway, I can send a follow-up PR when I confirm it isn't necessary anymore. For now, better safe than sorry 😄
There was a problem hiding this comment.
I tested it with the following diff:
diff --git a/packages/block-library/src/column/save.js b/packages/block-library/src/column/save.js
index c43edc8464..ccf1b64387 100644
--- a/packages/block-library/src/column/save.js
+++ b/packages/block-library/src/column/save.js
@@ -18,8 +18,7 @@ export default function save( { attributes } ) {
let style;
if ( width ) {
- // Numbers are handled for backward compatibility as they can be still provided with templates.
- style = { flexBasis: Number.isFinite( width ) ? width + '%' : width };
+ style = { flexBasis: width };
}
return (
diff --git a/packages/block-library/src/columns/variations.js b/packages/block-library/src/columns/variations.js
index 9f20699dee..6026a5f51e 100644
--- a/packages/block-library/src/columns/variations.js
+++ b/packages/block-library/src/columns/variations.js
@@ -74,8 +74,8 @@ const variations = [
</SVG>
),
innerBlocks: [
- [ 'core/column', { width: '33.33%' } ],
- [ 'core/column', { width: '66.66%' } ],
+ [ 'core/column', { width: 33.33 } ],
+ [ 'core/column', { width: 66.66 } ],
],
scope: [ 'block' ],
},and it seems to work correctly. Do you think we should update codebase or do I miss some other case?
There was a problem hiding this comment.
Tested this by adding a 30/70 columns block, and both columns are rendering with the same size. But that happens with or without your change to save.js, so looking for the issue now 🤔
There was a problem hiding this comment.
Scratch that, I was passing the template widths in as unitless strings 😅 . That's not something we'd ever expect to happen, is it?
I think it's fine to make this change, anyway!
There was a problem hiding this comment.
Yes, it works properly only with numbers. I had the same issue with strings. We can always check the issue filed and previous setup before it was converted to percentages. If it worked with strings in the past, we might need another fix 😃
gziolo
added
the
Backport to WP 6.7 Beta/RC
* Fix issues with different units in column widths * Columns with fixed width should keep width on recalculation * Address review feedback
* Cover block: Restore default overlay background (#26569) * Restore default Cover overlay background The default Cover block overlay background was removed. This changed the style of existing blocks on existing sites. Restore the default background in such a way that it does not conflict with theme-provided background-color styles and there is no need to artificially add extra specificity. Fix regression: #26545 * Limit the interface skeleton to a max width of 100% to prevent some blocks managing to exceed the width (#26552) * Cover Block: Restore opacity controls (#26625) * Fix image block centering when resizing to a smaller size (#26376) * Fix image block centering when resizing to a smaller size * Revert previous 100% width fix * Remove display: table when image block is resized to avoid centering of block * Match frontend classes for alignment in editor * Gallery: Remove caption fallback for alt attribute (#26676) * Fix quote block default alignment (#26680) * Gallery: Remove obsolete deprecation entry (#26736) * Do not apply text color if it is being overriden (#24626) * Fix: Preset colors don't work on button block style outline (#26707) * Tests: Add fixture for Column deprecation (#26774) * Fix/column width units (#26757) * Fix issues with different units in column widths * Columns with fixed width should keep width on recalculation * Address review feedback * fix undefined index notice in Social Link Block (#25663) * fix undefined index notice * use isset instead of array_key_exists check * Update packages/block-library/src/social-link/index.php Co-authored-by: Greg Ziółkowski <[email protected]> Co-authored-by: Greg Ziółkowski <[email protected]> * Adds in missing styling for toolbar when using text-only setting (#26769) * Adds in missing styling for toolbar when using text-only setting * Increases margin * Moves style to correct file * Inserter: Fix 'Browse All' in Quick Inserter (#26443) * Inserter: Fix 'Browse All' in Quick Inserter Maintain the insertion point in @wordpress/block-editor state when moving from the Quick Inserter to the Inserter. This fixes a bug where the insertion point is wrong after clicking a block appender and selecting 'Browse All'. Care is taken to not regress a previous bug where the insertion point is wrong after clicking an inline insertion button and selecting 'Browse ALl'. * Inserter: Fix typo Co-authored-by: Daniel Richards <[email protected]> * Call getBlockInsertionPoint once * Add useSelect dependencies * Make setInsertionPoint unstable * Avoid setting `isVisible` state when `SET_INSERTION_POINT` is triggered * Make index required and clarify rootClientId usage * Split insertionPoint into two reducers * Fix getInsertionPoint selector that expects default state of reducer to be null * Avoid resetting insertionPoint state on HIDE_INSERTION_POINT Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Jon Surrell <[email protected]> Co-authored-by: Jacopo Tomasone <[email protected]> Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Bernie Reiter <[email protected]> Co-authored-by: Greg Ziółkowski <[email protected]> Co-authored-by: Oliver Juhas <[email protected]> Co-authored-by: Jorge Costa <[email protected]> Co-authored-by: Fabian Kägy <[email protected]> Co-authored-by: Tammie Lister <[email protected]> Co-authored-by: Robert Anderson <[email protected]>
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
Description
Fixes #26096 and #26641
#24711 introduced two issues when column width attributes switched from being defined as numbers to strings:
This PR:
%to the end of a column width when it is defined as a number.To avoid breaking columns already set with fixed widths, recalculation on adding/removing columns only takes into account the existing column widths if they are defined in percent, or if they are unitless (which should be interpreted as percent for back compat reasons).
How has this been tested?
Tested in browser:
pxdon't get changed when adding or removing columns.%.Also added a few unit tests for the changes in the utility functions.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: