Skip to content
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

[Columns] Width listed in px rather than percentage #34096

Closed
2 tasks done
annezazu opened this issue Aug 16, 2021 · 7 comments · Fixed by #34768
Closed
2 tasks done

[Columns] Width listed in px rather than percentage #34096

annezazu opened this issue Aug 16, 2021 · 7 comments · Fixed by #34768
Assignees
Labels
[Block] Columns Affects the Columns Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@annezazu
Copy link
Contributor

Description

When adding in columns, the width is listed as px rather than percentages.

Step-by-step reproduction instructions

  1. Head to Site Editor.
  2. Add a Columns block.
  3. Select 70/30 option.
  4. Use list view to select an individual Column block and check the sidebar settings to view the width.
  5. See the width is set to px rather than percentages.

Screenshots, screen recording, code snippet

column.percentage.mov

Environment info

  • WordPress 5.8, GB 11.2.1, TT1 Blocks
  • Chrome
  • Desktop

Pre-checks

  • I have searched the existing issues.
  • I have tested with all plugins deactivated except Gutenberg.
@annezazu annezazu added [Type] Bug An existing feature does not function as intended [Block] Columns Affects the Columns Block labels Aug 16, 2021
@walbo
Copy link
Member

walbo commented Aug 17, 2021

Seem there is an issue in themes that customize the spacing units. The TT1 theme does this here: https://github.com/WordPress/theme-experiments/blob/10dc98f4bb3848d3a70078a165668572992e57e1/tt1-blocks/theme.json#L178-L187

"spacing": {
	"customPadding": true,
	"units": [
		"px",
		"em",
		"rem",
		"vh",
		"vw"
	]
},

Note that the TT1 theme doesn't have % in the allowed unit

When inserting the columns variations the width attributes is set in percentages. Thats why they initially are shown correctly but not allowed to edit as percentage.

innerBlocks: [
[ 'core/column', { width: '33.33%' } ],
[ 'core/column', { width: '66.66%' } ],
],

Not sure the best approach to solve this issue. Should percentages always be allowed in column block regardless of units in theme.json?

@annezazu
Copy link
Contributor Author

Oh wow! Thanks for this excellent sleuthing. Going to add in more labels to expand this conversation.

@annezazu annezazu added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label Aug 17, 2021
@ramonjd
Copy link
Member

ramonjd commented Aug 29, 2021

This is an interesting problem.

It's especially noticeable when trying to reset a column's width values:

columns.mp4

If I change the values/units, then remove them I can't return to the original width values and have to create a new column.

I wonder what our options are. I'm just rattling off some un-informed ideas to get things started.

If block variations natively require a specific unit, where should the compromise lie? Should the block itself enforce the use of % regardless of theme? Or should the variation follow the units defined in theme.json?

Allow block.json to define 'always-on' unit value

If we allow the block to tell theme.json what to do, we could perform a merge of customUnits, for example, using the arguments for useCustomUnits (so the columns block.json can define an immutable % unit)?

❗ I'm not sure the benefit would outweigh the obvious con, that is, it would dilute the authority of theme.json.

Restore initial variation value

Maybe a lower barrier would be to store the initial value, e.g., 33.33% regardless of whether the theme supports percentages, and restore it when we reset the control (requires the ToolsPanel. See also related PR: Block Support: Add width block support feature #32502)

Transform

Could we create custom transforms or variations depending on the unit value.

Perhaps in the onSelect callback of __experimentalBlockVariationPicker

This assumes of course that we have access to the custom units, and can check them against the units required by variations.js, AND then come up with a suitable replacement value depending on the unit. For example, 33.33% to {n}px.

This seems like the most work.

I'd be very happy to learn about better ways to approach this!

@andrewserong
Copy link
Contributor

If we allow the block to tell theme.json what to do, we could perform a merge of customUnits, for example, using the arguments for useCustomUnits (so the columns block.json can define an immutable % unit)?

That's a compelling idea @ramonjd — it feels like the Columns block has been built expecting that the percentage unit will be available, so I quite like the idea of a block being able to define an immutable unit. I wonder what the consequences would be for theme authors? In the case of TT1-blocks, where the units value is set globally, having an individual block take care of its own required units means that the theme author doesn't have to worry about potentially breaking block behaviour, so it could be beneficial, even if it's unexpected?

@richtabor
Copy link
Member

It's especially noticeable when trying to reset a column's width values:

I've run into this issue as well — resetting to px when initially set to %.

@andrewserong
Copy link
Contributor

I'm just assigning myself to this one, I thought I'd see if I can come up with a fix for the Column block when the width is set to a value that isn't supported by the spacing.units value of the current theme's theme.json (e.g. percentage widths).

@andrewserong andrewserong self-assigned this Sep 13, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Sep 13, 2021
@andrewserong
Copy link
Contributor

I have a PR ready for review in #34768 that attempts to deal with this issue by ensuring that the UnitControl will always display the current value's unit (if it's a valid CSS unit) irrespective of the theme's list of allowed units. If a theme provides a more limited list of available units, then once the unit of the current value is switched away from the not-allowed-unit, then the provided list is respected. I think this should resolve the issue for the Columns block requiring percentage widths for its variations, while also ensuring that we don't run into similar issues with other blocks.

Very happy for feedback, though, if folks think there's a better way to handle it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Columns Affects the Columns Block [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants