[RNMobile] Adjust Range-Control to add the stepper control and allow type selection #21465
Conversation
chipsnyder
added
the
Mobile App - i.e. Android or iOS
chipsnyder
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta and
youknowriad
as code owners
|
Size Change: +1.77 kB (0%) Total Size: 905 kB
ℹ️ View Unchanged
|
|
Thanks for taking this task @chipsnyder ! I think we are going in the right direction except for some things, I'll add them as a list but please let me know if some don't make any sense, it's what I've gathered from the discussions related to this component.
As I said before let me know if it makes sense or not 😅 |
|
Thanks @geriux for the review. This is a great background on when these controls were added and why. I made those suggested changes and it's ready for your next review! |
| label={ __( 'Columns' ) } | ||
| value={ columns } | ||
| onChange={ this.setColumnsNumber } | ||
| min={ 1 } | ||
| max={ Math.min( MAX_COLUMNS, images.length ) } | ||
| { ...MOBILE_CONTROL_PROPS } | ||
| type="stepper" |
There was a problem hiding this comment.
This prop is for mobile only so what if we move it to MOBILE_CONTROL_PROPS? I know that it will be removed in the future but until then it looks like it's a good place, what do you think?
There was a problem hiding this comment.
My initial thoughts were adding it to MOBILE_CONTROL_PROPS might be confusing since it only applies to RangeControl. But you're right this might not be the best place for it because it will probably have the same problem as seperatorType. I can move that up to MOBILE_CONTROL_PROPS with some comments on why its there
There was a problem hiding this comment.
Oh right! Totally missed that is being used for the ToggleControl as well. So I'm guessing we could use the Platform check inline, if it's native, pass down the prop.
There was a problem hiding this comment.
it will probably have the same problem as
seperatorType
Actually, we are "lucky" here, in that the web implementation passes the props down to <InputRange> but explicitly sets type after the spread props:
There was a problem hiding this comment.
Hey @geriux
I failed to address @mkevins concerns in his comment here. (Sorry again about that Matt 😅 So just wanted to make sure you were in the loop as well.
Here is part of our convo from Slack that I think picks up the conversation well
Hey Chip 👋 😄
So, for some context: #20231 (comment)
There is actually some concern voiced over the use of this pattern with the MOBILE_CONTROL_PROPS platform deviating variable (which I introduced). This was introduced to deal with some platform differences that are cumbersome to reconcile. In the case for the type prop, I think it'd be best if we worked with web to develop a standard way to deal with "platform specific" props for a shared component. I.e. maybe the ideal solution is that we add this prop to web RangeControl to officially consume it, so it's not part of the ...props rest parameter when passed on to children. In the meantime, IMO it'd probably be best to limit the use of that pattern to where it's needed.
There is a lint-ignore that web has used a bit in the past for consuming unused props (heavily used in RichText, irrc), and now I'm wondering whether we can make this nicer.. perhaps via destructured rename with a particular prefix.. :thinking_face:
I think there are two parts here. What should we do for this case (I would be happy to make any changes). Then also what should we do long term for these patterns.
Focusing on the first part "What should we do for this case?" @geriux what do you think about @mkevins concerns?
There was a problem hiding this comment.
Yeah, I do agree that MOBILE_CONTROL_PROPS isn't the best approach, we did make an improvement already by removing the StepperControl and moving the rendering logic to the RangeControl (mobile).
Actually, we are "lucky" here, in that the web implementation passes the props down to but explicitly sets type after the spread props:
I talked about this with Chip and we also saw that we could just pass type and it wouldn't affect Web's side when rendering the component but it'd be weird if it was just being used by the mobile version of the component.
What should we do for this case?
I agree with Matt to add type to both RangeControl but I'm not sure how the web component could benefit from this prop. Maybe use already existing props? For example, I saw there's withInputField and the stepper doesn't have an input field since it's for smaller values. Maybe instead of having type=stepper we could pass withInputField=false (for mobile) to render the Stepper instead of the Slider?
There was a problem hiding this comment.
stepper doesn't have an input field since it's for smaller values
That rule is kinda outdated. Columns have now no limit for column count and it is controlled by stepper. Plus, our top priority should be keeping the component api meaningful and clear. If I were to use this component as a block author it wouldn't be clear to me that "withInputField=false" renders a stepper. I think the best way is to keep the "type" prop but document it as mobile only for now(which we already did) and if web folks would like to utilize it they can use it to render a web version of the stepper as well. There are also some props that are web only. I think the best way is just ignoring/discarding the mobile specific prop inside the web implementation. Otherwise, needing to filter passed props via platform is not a good development experience for the block author and it is quite easy to miss.
There was a problem hiding this comment.
I don't have a strong opinion short-term, but in general I see three options:
- Continue using this
MOBILE_CONTROL_PROPSpattern until a longer-term solution lands - Use the
Platform.selectfunction directly within the jsx - Rely on the "luck" that
typeis currently being overwritten after the...propsrest parameter
The third option feels the most risky, since I don't know how likely it is the type prop would be moved / removed in the parameter list.
Longer term, it seems we may encounter a common situation whenever we have a cross-platform component with a few "platform-specific" props. One way to handle this is to explicitly name the unused prop, which removes it from the rest parameter. This then also requires a lint-ignore line, and usually an explanation for the lint-ignore (e.g. this is used in ClipboardButton component).
The eslint rule no-unused-vars has an option for ignoring rest parameter siblings, which is nice, but likely too crude for this purpose (since it would affect other unused props). There is also a convenient argsIgnorePattern option which allows us to ignore variables that match a specified RegEx.
Perhaps we can utilize this along with a destructuring rename, to create a pattern that makes it clear which props are ignored as platform specific, while also being removed from the rest parameter, and without the need for too many lint-ignore messages.
Example patch
diff --git a/.eslintrc.js b/.eslintrc.js
index a4a1ec153e..194e484307 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -121,6 +121,10 @@ module.exports = {
'Avoid truthy checks on length property rendering, as zero length is rendered verbatim.',
},
],
+ 'no-unused-vars': [
+ 'error',
+ { argsIgnorePattern: '^__mobileOnlyProp' },
+ ],
},
overrides: [
{
diff --git a/packages/block-library/src/gallery/edit.js b/packages/block-library/src/gallery/edit.js
index 3f77212f71..0cc920819c 100644
--- a/packages/block-library/src/gallery/edit.js
+++ b/packages/block-library/src/gallery/edit.js
@@ -65,11 +65,6 @@ const MOBILE_CONTROL_PROPS_SEPARATOR_NONE = Platform.select( {
native: { separatorType: 'none' },
} );
-const MOBILE_CONTROL_PROPS_RANGE_CONTROL = Platform.select( {
- web: {},
- native: { type: 'stepper' },
-} );
-
class GalleryEdit extends Component {
constructor() {
super( ...arguments );
@@ -403,7 +398,7 @@ class GalleryEdit extends Component {
min={ 1 }
max={ Math.min( MAX_COLUMNS, images.length ) }
{ ...MOBILE_CONTROL_PROPS }
- { ...MOBILE_CONTROL_PROPS_RANGE_CONTROL }
+ type="stepper"
required
/>
) }
diff --git a/packages/components/src/range-control/index.js b/packages/components/src/range-control/index.js
index d718dec1a2..c9dccefffd 100644
--- a/packages/components/src/range-control/index.js
+++ b/packages/components/src/range-control/index.js
@@ -64,6 +64,7 @@ const BaseRangeControl = forwardRef(
renderTooltipContent = ( v ) => v,
showTooltip: showTooltipProp,
step = 1,
+ type: __mobileOnlyPropType,
value: valueProp,
withInputField = true,
...propsThis pattern works, but it still isn't perfectly clear that it is a mobile-only prop where it is used. Instead, it's only clear once visiting the component code. A way to potentially resolve that is to forgo the destructuring rename, and add a prefix to the prop itself to denote it's platform specificity: __mobileOnlyPropType="stepper". The same lint-ignore pattern could be used, and it would be clear in both the implementation and the usage of the component that the prop in question was a platform specific one.
In the solution which uses the prefixed variable directly, the platform specific implementation that actually uses it could use destructuring rename to aid readability for consuming code:
// my-component.native.js
const MyComponent = ( {
__mobileOnlyPropSomeProp: someProp,
someOtherProp,
...props
} ) => (
someProp === 'someValue'
? <SomeChild { ...props } />
: <SomeOtherChild { ...props } />
);I would love to hear some other thoughts about this pattern as well, or other ideas / patterns for how to bring clarity to platform differences in our shared code. :)
There was a problem hiding this comment.
Thank you for giving more detailed options @mkevins!
I think first I'd try to go with option 3:
Rely on the "luck" that type is currently being overwritten after the ...props rest parameter
As @pinarol mentioned, it is in the docs that it is a mobile-only prop. But I know it's easy to miss by only looking at the code.
I really like those patterns you suggested though, maybe it is worth investigating for future implementations that are mobile-only and make it some sort of a standard in the code.
Hey @chipsnyder! thanks for the changes! I commented on some small things but it's looking good! |
Co-Authored-By: Gerardo Pacheco <gerardo.pacheco@automattic.com>
…ss/gutenberg into rnmobile/issue/1992-RangeControl
|
Great feedback on those comments @geriux, those changes are done now. |
There was a problem hiding this comment.
LGTM! Thank you for all the changes and for working on this @chipsnyder!
Tested on both iOS, Android and local instance of the web editor. ✅
Fixes: wordpress-mobile/gutenberg-mobile#1992
gutenberg-mobilewordpress-mobile/gutenberg-mobile#2125Description
This PR refactors the RangeControl component to allow the optional parameter
typeto allow selection between the Stepper control or a slider control on Mobile.Since the range control is being used in multiple places right now as a slider control, I made the slider the default option.
This PR also updates the gallery component to pass in
type=stepperto test the other use flow. The gallery component was also the only place where I could find references to the stepper control.How has this been tested?
Stepper:
Slider:
Open a control using the RangeControl. Examples:
Expect to see the slider control
Screenshots
Types of changes
New feature adds type to RangeControl
Checklist: