Hey, @mkevins 👋 Thanks for the feedback and for suggesting having the same props in the Stepper component so we can reduce the code for the Gallery. Since that component is not used yet it was the best time to update it. Feel free to review it again when you can.

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 STEP_SPEED could be adjusted, or if a separate initial delay can be added to avoid debounce the first tap (it was hard to get it not to jump 2 at a time, for example).

Also, not related to this PR, I noticed something in the code for stepper component:

On this line:

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!

it seems that 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):

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

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

Sounds good to me! I'll change it since I'm fixing the argument issue.

@mkevins Updated with the feedback =)

I am seeing warnings that I suspect are related to this pull request, at least when running npm run storybook:dev.

[1] WARNING in ./packages/block-library/build-module/gallery/edit.js 70:60-74
[1] "export 'StepperControl' was not found in '@wordpress/components'
[1]  @ ./packages/block-library/build-module/gallery/index.js
[1]  @ ./packages/block-library/build-module/index.js
[1]  @ ./storybook/stories/playground/index.js
[1]  @ ./storybook/stories sync ^\.\/(?:(?:|\/|(?:(?:(?!(?:|\/)\.).)*?)\/)(?!\.)(?=.)[^/]*?\.(js|mdx))$
[1]  @ ./storybook/generated-entry.js
[1]  @ multi ./node_modules/@storybook/core/dist/server/common/polyfills.js ./node_modules/@storybook/core/dist/server/preview/globals.js ./node_modules/@storybook/addon-docs/dist/frameworks/common/config.js ./node_modules/@storybook/addon-docs/dist/frameworks/react/config.js ./storybook/preview.js ./node_modules/@storybook/addon-knobs/dist/preset/addDecorator.js ./storybook/generated-entry.js (webpack)-hot-middleware/client.js?reload=true&quiet=true

My guess is that this import is problematic in that there is no StepperControl implemented for web, so it cannot be imported:

Separately, I would have some concern that we've introduced a diverging feature between mobile and web implementations (i.e. StepperControl in native but not web).

Hi @aduth 👋

I am seeing warnings that I suspect are related to this pull request

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 StepperControl, but it'd be good to reduce the noise on the dev side if possible. I'm guessing the warning is thrown by one of the Webpack loaders? Not sure offhand what may be the best way to suppress that, perhaps via a stub 🤔 ?

Separately, I would have some concern that we've introduced a diverging feature between mobile and web implementations (i.e. StepperControl in native but not web).

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 !

Separately, I would have some concern that we've introduced a diverging feature between mobile and web implementations (i.e. StepperControl in native but not web).

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. StepperControl is mobile-only, so it can't be used in edit.js.