[RNMobile] - Gallery Block: Columns Control

[geriux]

Gutenberg Mobile PR -> wordpress-mobile/gutenberg-mobile#1905

Description

Due to this PR #19947 a warning started to show up due to a missing import for web.

I'm proposing to add a ColumnsControl component with its own Web and Mobile variant.

The Web variant will have the RangeControl and the Mobile will have the StepperControl.

Having two different components (Slider / Stepper) was a design decision to improve the UX on mobile.

Any other solution for this case is more than welcome =)

How has this been tested?

Web:

• Start docker
• Open a new post
• Add a Gallery block
• Select images
• Open Settings
• Play with the slider in Columns section
• Expect the value to change and the Gallery to adjust.

Mobile:

• Open the demo app with metro running
• Add a Gallery block
• Select images
• Open Settings
• Change the value of the stepper
• Expect the value to change and the Gallery to adjust (Switch to landscape to see more columns)

Types of changes

Bug fix (Import Warning)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[aduth]

One thing I like about this is that it makes it clear that it will be painful to use a native- or web-exclusive component. I would like that to be as painful as possible to discourage the practice, because I would prefer there to be few (ideally zero) differences in the component set we offer between web and mobile.

Related to aforementioned design decision, statements like "stepper is better than a range when fewer options exist" do not seem to me as exclusive to one or the other of mobile or web, so I'm still not entirely convinced this should be a native-only component.

[aduth]

The problem with this sort of spread is that it makes it unclear to me now in this moment of refactoring whether we can remove the MOBILE_CONTROL_PROPS object or any of its properties. I see that it is spread into other components, but I have no idea what (or if any) of the properties are being used in those components.

I would have preferred that each prop were individually listed, even if that is more verbose, to make it clearer what is needed and where.

Or better yet, and to the same point as #20231 (comment), we aim to avoid any differences in the component API, so that we don't need to do this at all.

[mkevins]

Hi @aduth 👋

This current implementation with the MOBILE_CONTROL_PROPS object (as well as the even uglier MOBILE_CONTROL_PROPS_SEPARATOR_NONE object introduced here) is admittedly a pretty hacky workaround. This in part arises from differences in how controls are displayed on web and native (on native, PanelBody is in the bottomsheet).

There are at least two areas of technical debt that prompt this awkwardness:

1. Native implementations of various control components have different default separatorTypes.
2. The responsibility of rendering (or not) the separator currently belongs to the Cell component, from which all mobile control components are composed.

Proposed solutions:

Due to 1., we must "manually" specify the separator types to ensure consistency in a group of controls (not ideal). Instead, the controls should have a common default separatorType obviating the need for this hack. Due to 2., the responsibility of handling fence-post issues for the separators is left to the parent components. It might make more sense to shift this responsibility to the native implementation of PanelBody, or if not there, a container component to abstract it away and avoid fence-post logic "leaking" into the parent views.

In the current implementation, these spreadables were extracted to the top per this discussion: #18265 (comment), however, you raise an interesting point about the "painfulness" signalling. Having each prop verbosely listed (with appropriate comments included) could serve to amplify the existence of the underlying technical debt (which is currently signaled only by the code comment).

[aduth]

Yeah, I don't exactly understand how this needs to behave in mobile with showing in the bottom card, but it does not seem to me that this separator ought to be a concern of the *Control components, which are otherwise labeled input controls agnostic to where they occur in the page. If it's a consequence of how these are rendered within PanelBody, then I would agree this should be managed by PanelBody (either by styling, or perhaps in its render behavior in inspecting/iterating its props.children).

[mkevins]

Related to aforementioned design decision, statements like "stepper is better than a range when fewer options exist" do not seem to me as exclusive to one or the other of mobile or web, so I'm still not entirely convinced this should be a native-only component.

I agree with this, and it may be a good candidate for code sharing too. Though, I think perhaps some name changes to a shared common component might be useful, since IIRC, "stepper" can mean a different thing on web (actually on Android as well): a task-driven, or "checkout page" kind of UX.

[koke]

I'm still not entirely convinced this should be a native-only component.

I think conceptually StepperControl is similar to TextControl with type=number, but the web version inherits from HTML and keeps it in there.

Also, I think it's worth revisiting what RangeControl means, since it doesn't necessarily have to equate Slider. Semantically, both components allow you to pick a value from within a range, but you would use a stepper when the range is small or you care about the exact numeric value, and a slider when the value is more "analog" or there's a much wider range. Oddly enough, the web versions seems to be actually using both:

I think there's room for RangeControl to be smarter and support both variants depending on configuration.

[pinarol]

Related to aforementioned design decision, statements like "stepper is better than a range when fewer options exist" do not seem to me as exclusive to one or the other of mobile or web, so I'm still not entirely convinced this should be a native-only component.

@aduth I am very interested in hearing more about the unmet expectation here. Do you mean mobile team should've implemented the web equivalent of Stepper as well? Or we should have avoided to add the native only Stepper in the first place?

[aduth]

@pinarol For me, it's a bit of both of what you mention. We should want to avoid adding components which target only one or the other of web or native. And if we do implement one, to at least have some interoperability in place to minimally support the component in both environments.

I really like this bit from @koke's comment:

Also, I think it's worth revisiting what RangeControl means, since it doesn't necessarily have to equate Slider.

Originally, I had been thinking that maybe Stepper could have a web implementation which simply renders the existing RangeControl component, since it serves as the web's solution for what Stepper aims to implement in native (at least for the moment).

But to @koke's point, the underlying interface of a component doesn't need to be so prescriptive about the specific UI it creates. If the purpose of RangeControl is to give a user an option to select within a range of values, and in mobile we want to use a step control for when fewer options exist, it doesn't seem problematic to me that this rendering behavior be chosen programmatically, and dynamically based on the input props of the component.

In other words, would it be reasonable to eliminate StepperControl and absorb the logic of rendering a step control into the existing RangeControl?

[pinarol]

In other words, would it be reasonable to eliminate StepperControl and absorb the logic of rendering a step control into the existing RangeControl?

Yes, I think it would be reasonable for the cases we have currently. Not sure what future will bring but we can always iterate later. I have opened a tech debt issue to address this and added to our project board. We'll try to take care of this in the following ~2 sprints. Thanks!

Same for the separatorType issue, we shouldn't delay it much.

BTW, sharing the same component library has been in our radar for quite some time. We worked on “cross-platformitization” of components like: InspectorControls, ToggleControl, SelectControl, RangeControl, MediaPlaceholder, MediaUpload... ~6 months ago none of them shared the same props, also some of them behaved differently. We refactored them to unify props between platforms. We have been also working on x-platform primitive components so that we can define a common style system between web & mobile.
Box primitive: #17614
Button primitive: #19104
Having a common component library is something we are already working on, but other priorities can get in the way and delay some part of the work from time to time. But at the end I believe we have the same vision, that's why actually we tried making the mobile gallery share code with web as much as possible, to get closer to web. It is the first time we are sharing a rather complex "UI" code with web.

[gziolo]

In other words, would it be reasonable to eliminate StepperControl and absorb the logic of rendering a step control into the existing RangeControl?

I had the same objections when StepperControl was proposed. In general branching code based on the UI aspects is problematic as it makes using components harder. You have to add another layer of indirection in the place where they are consumed. This PR fixes also an issue where StepperControl was imported for web implementation but it was never exposed when using from npm.

[pinarol]

I had the same objections when StepperControl was proposed.

@gziolo Could you point me to those objections? If there's anything that we ignored I'd love to do a retro about why it happened that way with the team. We always try to include web folks in such PRs, as far as I see Stepper control was merged with this PR and I see that you asked a question, and sorry if I mistranslated it but that doesn't seem like an objection to me. If there's something else please let us know.

[gziolo]

I had the same objections when StepperControl was proposed.

@gziolo Could you point me to those objections? If there's anything that we ignored I'd love to do a retro about why it happened that way with the team. We always try to include web folks in such PRs, as far as I see Stepper control was merged with this PR and I see that you asked a question, and sorry if I mistranslated it but that doesn't seem like an objection to me. If there's something else please let us know.

Yes, it looks like this comment and it doesn't look like strong "no" 😄 I don't want to block the work only because I have a different opinion on how to approach it. To be clear, I'm not against having dedicated components for mobile UI but we should always try to move the web implementation in the direction that helps components to be cross-platform by keeping higher-level API that makes it easier to consume code.