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

Add minimum and maximum gallery columns attribute #16314

Merged

Conversation

@spacedmonkey
Copy link
Contributor

spacedmonkey commented Jun 26, 2019

Make it clearer when define the gallery block, min/max and default values.

Copy link
Member

jorgefilipecosta left a 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,

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 4, 2019

Member

JSON schema uses "minimum" and "maximum" https://json-schema.org/understanding-json-schema/reference/numeric.html#integer, I guess should use the same properties.

This comment has been minimized.

Copy link
@spacedmonkey

spacedmonkey Jul 4, 2019

Author Contributor

Min and max are used elsewhere in the code base.

"min": 0,
"max": 100

This comment has been minimized.

Copy link
@spacedmonkey

This comment has been minimized.

Copy link
@aduth

aduth Jul 5, 2019

Member

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.

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 17, 2019

Member

@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

This comment has been minimized.

Copy link
@aduth

aduth Dec 17, 2019

Member

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

This comment has been minimized.

Copy link
@aduth

aduth Jan 6, 2020

Member

Follow-up pull request including the typo correction and numeric ranges validation: #19433

@spacedmonkey

This comment has been minimized.

Copy link
Contributor Author

spacedmonkey commented Jul 4, 2019

The reason for this PR, is that these values are found elsewhere in the codebase but should be defined.

default, of 3 -

return Math.min( 3, attributes.images.length );

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.

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Nov 2, 2019

Can we get a status update on this PR?
@spacedmonkey @jorgefilipecosta @youknowriad

@jorgefilipecosta jorgefilipecosta force-pushed the spacedmonkey:fix/default-gallery-values branch from e615038 to 239694b Jan 3, 2020
@jorgefilipecosta jorgefilipecosta force-pushed the spacedmonkey:fix/default-gallery-values branch from 239694b to 01c030a Jan 3, 2020
Copy link
Member

jorgefilipecosta left a 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 jorgefilipecosta changed the title Improve gallery block defaults Add minimum and maximum gallery columns attribute Jan 3, 2020
@jorgefilipecosta jorgefilipecosta merged commit a941c49 into WordPress:master Jan 3, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.