Prevent entering invalid values in the Query Loop block config #33285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Bug
|
Size Change: -2 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ntsekouras
approved these changes
Jul 8, 2021
There was a problem hiding this comment.
Let's go with this one that checks other inputs as well. Although the crashing comes only from perPage due to the logic for initial setup of this attributes, which should be handled better in this issue.
Thanks Riad
sarayourfriend
requested changes
Jul 8, 2021
There was a problem hiding this comment.
To match the HTML API, the min and max should be inclusive of the validated value, not exclusive as you have it here.
Demo here: https://codesandbox.io/s/empty-platform-h2qvj?file=/src/App.js
Also I created an issue to add a baseline validation based on the min/max props: #33291
|
I think they are inclusive in this PR (unless I'm misunderstanding). The codepen link seems wrong. |
sarayourfriend
approved these changes
Jul 8, 2021
|
It seems like the gutenberg/packages/components/src/number-control/test/index.js Lines 68 to 96 in dacdf41 Ahhhh but it doesn't "clamp" on blur, only on key down 🤔 |
|
@sarayourfriend interesting! even in keydown (which I didn't experience) it sounds weird/wrong to me, what if the limit is 100 < x < 200 and I type something like For 5.8, I'm going to land this stop gap solution and we should fix this properly in #33291. |
Oh it's only on the Enter key down, not any key down. So you can finish your number being input for sure, but when you hit enter it will "clamp" the value. It even seems like it's meant to do the same thing on blur as well: gutenberg/packages/components/src/input-control/input-field.js Lines 92 to 109 in ad0cc77 Notice how it calls In any case, this will take some digging to discover the correct solution to it. It doesn't seem like it should take much new code to fix this problem. |
youknowriad
force-pushed
the
fix/query-toolbar
branch
from
6c07e2b to
ba43743
Compare
youknowriad
force-pushed
the
fix/query-toolbar
branch
from
ba43743 to
161e000
Compare
youknowriad
added a commit
that referenced
this pull request
Jul 12, 2021
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
added a commit
that referenced
this pull request
Jul 13, 2021
sarayourfriend
pushed a commit
that referenced
this pull request
Jul 15, 2021
12 tasks
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #33270
Right now, you can enter
0in the Query Loop per page config which will result in an infinite loop breaking the block.This PR fixes that by adding some validation to the Query Loop Config inputs.
This more a stop gap fix for 5.8. I think a better solution would be to do that validation automatically in the
NumberControlcomponent. I tried this quickly but felt a bit impactful for 5.8 given how complex that component looks (not sure if that complexity is needed tbh) so I think I'd rather leave this for our components devs if you all have time, what do you think @ciampo @sarayourfriend.Testing instructions