Add minimum and maximum gallery columns attribute #16314
Conversation
spacedmonkey requested review from ajitbohra, gziolo, jorgefilipecosta, notnownikki, Soean, talldan and youknowriad as code owners
mtias added the [Block] Gallery 
There was a problem hiding this comment.
Hi @spacedmonkey thank you for your contribution 👍
These values have no impact on the parsing e.g.: If I execute: wp.blocks.parse('<!-- wp:gallery {"columns":10} -->!-- /wp:gallery -->' ); the attribute collumns will contain the value of 10 anyway.
I wonder if we should have this indication even if it is not enforced in any way. I guess the advantage of this change is that it would make the server aware of these restrictions. Are there other advantages? Any thoughts on this: @aduth, @youknowriad?
We could also extend the PR/follow up on it, and add this validation to the parser, but that also raises some questions:
- Is the added complexity of this validation worth it?
- JSON schema supports other restrictions on numbers e.g: multipleOf, what differentiates the restrictions we support from the restrictions we don't support?
| "type": "number" | ||
| "type": "number", | ||
| "default": 3, | ||
| "min": 1, |
There was a problem hiding this comment.
JSON schema uses "minimum" and "maximum" https://json-schema.org/understanding-json-schema/reference/numeric.html#integer, I guess should use the same properties.
There was a problem hiding this comment.
Min and max are used elsewhere in the code base.
gutenberg/packages/block-library/src/column/block.json
Lines 10 to 11 in 5494bc5
There was a problem hiding this comment.
The rest of code uses minimum and maximum
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api.php#L1227-L1267
There was a problem hiding this comment.
Huh, this hadn't occurred to me. I don't know that our JavaScript implementation enforces this validation. Generally, if the idea is to follow JSON Schema, we should probably conform to the proper names. I suppose we'd need to alias existing values to avoid breaking changes.
It seems like something we might want to consider addressing separate from this pull request, however.
There was a problem hiding this comment.
@aduth does it mean that in this PR we should use minimum and maximum, and as a follow up we should change the column block (and have a proper alias for back-compatibility)?
cc: @spacedmonkey
There was a problem hiding this comment.
@aduth does it mean that in this PR we should use minimum and maximum, and as a follow up we should change the column block (and have a proper alias for back-compatibility)?
Yes, let's conform to the intended names here ("minimum", "maximum"), regardless of consistency or validation support elsewhere.
There was a problem hiding this comment.
Follow-up pull request including the typo correction and numeric ranges validation: #19433
The reason for this PR, is that these values are found elsewhere in the codebase but should be defined. default, of 3 - max of 8 - min of 1 - I believe that correct that doesn't valid the max, the slider doesn't let you go higher than 8. |
Can we get a status update on this PR? |
jorgefilipecosta force-pushed the fix/default-gallery-values branch from e615038 to 239694b Compare
jorgefilipecosta force-pushed the fix/default-gallery-values branch from 239694b to 01c030a Compare
There was a problem hiding this comment.
I update this PR to use minimum and maximum as discussed. Regarding the default property I needed to revert it as it was causing some tests to fail. I think the gallery save needs to be updated to take into account the new default attribute value, we should deal with that in a follow-up PR.
jorgefilipecosta changed the title
Make it clearer when define the gallery block, min/max and default values.