-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] - Replace Slider with Stepper for columns settings on Gallery block #19947
Conversation
Hey, @mkevins 👋 Thanks for the feedback and for suggesting having the same props in the |
Hi @geriux 👋 This is looking good! Thanks for those changes 👍 I tested on both Android (via Pixel 3a, Android 10) and iOS (via simulator - iPhone 11). Works mostly as expected. One observation is that on Android, the "repeat rate" was pretty fast: I wonder if the Also, not related to this PR, I noticed something in the code for stepper component: On this line: gutenberg/packages/components/src/mobile/bottom-sheet/stepper-cell/index.native.js Line 88 in 7c0a10d
startPressInterval is invoked with a second argument (I assume a delay to "accelerate" repeats after 10 ticks). However, the function only seems to accept one argument (the callback): gutenberg/packages/components/src/mobile/bottom-sheet/stepper-cell/index.native.js Line 80 in 7c0a10d
The galleries I tested with did not have 10 or more images, so I didn't test the behavior, but I wanted to note that in case it's something worth taking another look at (also, if we need to change anything there, perhaps STEP_DELAY is a more accurate name than STEP_SPEED, e.g. when we divide it by 2, the speed actually should increase). Wdyt? |
Thanks for reviewing it again!
😱good thing you spotted this! Due to this missing argument, it was causing the other issues you mentioned about being too fast or an initial delay.
Sounds good to me! I'll change it since I'm fixing the argument issue. |
…/stepper-gallery-settings # Conflicts: # packages/block-library/src/gallery/edit.js # packages/components/src/mobile/bottom-sheet/stepper-cell/index.native.js # packages/components/src/mobile/stepper-control/index.native.js
@mkevins Updated with the feedback =) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested again via WordPress apps on Android (Pixel 3a) and iOS (simulator). Everything works as expected. Nice work @geriux 👍
LGTM!
…thub.com:WordPress/gutenberg into rnmobile/stepper-gallery-settings # Conflicts: # packages/components/src/mobile/stepper-control/README.md
I am seeing warnings that I suspect are related to this pull request, at least when running
My guess is that this import is problematic in that there is no gutenberg/packages/block-library/src/gallery/edit.js Lines 21 to 28 in c84784f
Separately, I would have some concern that we've introduced a diverging feature between mobile and web implementations (i.e. |
Hi @aduth 👋
Indeed, this PR would be the source of that warning. It shouldn't be problematic for users (since we use the Platform module to "fork" the code path which uses
I believe this was introduced to improve the UX on mobile for settings with small value ranges (i.e. < ~10), though I would defer that to @iamthomasbishop or @pinarol |
Hey @aduth !
As @mkevins mentioned, it was introduced as a UX improvement on mobile, you can read more about this decision here Regarding the warning, I'll put up a draft PR with a fix proposal. Thank you for the heads up on this! |
Here's the draft PR with the fix proposal #20231 any feedback / different approach is more than welcome =) Thanks! |
We published an update of WordPress packages to npm and this PR breaks Gallery block when used outside of Gutenberg. |
Gutenberg Mobile
PR -> wordpress-mobile/gutenberg-mobile#1834Description
This PR replaces the
RangeControl
that handles the number of columns for theGallery
block with the Stepper component only for mobile.It also changes the prop names of the
Stepper
component to match theRangeControl
for simpler use. Since this component is not being used until now it was the best time to do it.How has this been tested?
Gallery
blockScreenshots
Types of changes
Stepper
component props name to match theRangeControl
Checklist: