Open koke opened 4 years ago
cc @ItsJonQ, you may be interested in this.
@mcsf Thanks for the heads up!
@koke Awesome break down! And thank you for being so active with the discussions!
I’d want to explore how do we define styles for components within another container component...
I would love to as well! As an experiment, maybe we can explore how we can refactor a single component, maybe Panel
?, that can work well for web + native. With a balanced component API that's flexible and opinionated.
The tricky thing is, (for me), I don't know of all the ways we'd want something like Panel
to render and function for both platforms. It's like we're building as we go, haha.
Since we're starting to leverage Storybook more for web UI, I think it'll be handy to set it up for React Native as well.
I think I've chatted about React Native + Storybook briefly with @diegohaz.
I think the concerns you've raised is great!! <3
I completely agree. Any current interface should be considered ephemeral from the perspective of declaring the functionality of a block. The InspectorControls
component is a good example because it purposely tried to avoid location references (as in: "Block Sidebar") under the assumption that whether the controls are displayed in a sidebar or somewhere else (a modal, the mobile bottom sheet, etc) is irrelevant to the block author and is subject to change.
Panel
is one where the current interface has dictated too much of the inherent semantics of the components which is a mistake. The block author should be declaring which tools exist as "inspector tools" vs "toolbar tools". It's then up to the editor application to map the tools to the relevant interface elements (panels or something else) accordingly.
Having the mobile project running alongside is a great incentive to make sure the semantics are both general enough and meaningful enough, because we also don't want to create abstractions that end up being obscure.
InspectorControlsGroup
kind of makes sense, but I'm not sure I understand what InspectorControlsButton
would represent. Wouldn't a developer add a regular Button
either to the root of InspectorControls
or within InspectorControlsGroup
?
https://github.com/WordPress/gutenberg/pull/16384#issuecomment-545029533 bears similar stakes around the semantics—physicality debate.
Wouldn't a developer add a regular Button either to the root of InspectorControls or within InspectorControlsGroup?
@mtias I think that would be the ideal API, but is not clear to me how that would work in a CSS-in-JS solution yet
In styled-components
you can override styles in a very similar way to how we'd override styles in vanilla CSS, with similar pros and cons.
// button.js
export const Button = styled.button`
color: green;
`;
// inspector-controls.js
import {Button} from './button.js';
export const InspectorControls = styled.div`
${Button} {
color: red;
}
`;
Though I don't believe this functionality is supported by emotion
. /cc @diegohaz
I think the preferred approach is to use composition - the InspectorControlsButton
approach.
// button.js
export const Button = styled.button`
color: green;
`;
// inspector-controls.js
import {Button} from './button.js';
export const InspectorControls = styled.div``;
export const InspectorControlsButton = styled(Button)`color: red;`;
The semantics of a Button
being a Button
everywhere is nice, but, practically if the context of where a button is used dictates a slightly different usage, style or behaviour, then InspectorControlsButton
is a necessary pattern - particularly if you throw types into the mix!
As a theoretical example, in InspectorControls
we probably want to constrain the use of button props in order to maintain a consistent button sizing across all buttons in the group:
// we want consistent sizing
<InspectorControls size="large">
<InspectorControls.Button>A</InspectorControls.Button>
<InspectorControls.Button>B</InspectorControls.Button>
</InspectorControls>
// instead of this
<InspectorControls>
<Button isSmall>A</Button>
<Button isLarge>B</Button>
</InspectorControls>
There might be a bunch of other properties we might want to constrain on a button within InspectorControls
e.g. we might not allow the isLink
variation of buttons.
That makes perfect sense to me, and it aligns with what I'm used to seeing in mobile. As long as InspectorControls.Button
is implemented using Button
and reusing the same behavior that we have for the rest of buttons, it makes sense to add a new type of component to constraint the possible configurations to those that make sense within InspectorControls
.
Maybe I don't fully understand styled()
yet, but in your example, since InspectorControlsButton = styled(Button)
, wouldn't it still be possible to add a <InspectorControlsButton isSmall/>
?
Maybe I don't fully understand styled() yet, but in your example, since InspectorControlsButton = styled(Button), wouldn't it still be possible to add a
?
Yep apologies, that was an over-simplified example!
Here's a an example where I've implemented the pattern in recently, using context to pass down shared props like size
from the container component and limiting the set of props which can be set on the button itself.
https://github.com/salad-ui/components/blob/d3ee0b688bf01cc7e5954a9a7c0b556cd49e1123/packages/button/src/button-group/ButtonGroupButton.tsx
@koke Just checking in to see whether this is still an issue for you in testing.
@jordesign sorry, I haven't worked on gutenberg for the last few years, so I'm not sure if this is still an issue
I have growing concerns about several issues with our native versions of some components that I've spotted over several PRs recently. I can list the details later, but there are two main ones: Cells and Panels.
Cells: not every control is one
@wordpress/components
is about creating a set of UI elements that you can reuse anywhere. So far, every*Control
element that we have implemented for native, uses a variant of Cell.I understand this is because our current use case is putting these in a
BottomSheet
, but there's nothing about aTextControl
that says it will be used in aBottomSheet
. For instance, the rss block uses aTextControl
within the edit interface. Or the contact forms in Jetpack.TextControl
is a particularly interesting example because it's not even what the control is supposed to be: the native variant is more of a Button than a text input. But we also haveSelectControl
(usesPickerCell
),ToggleControl
(usesSwitchCell
), andRangeControl
(which seems like it skipped adding a native variant of the existing control and used theRangeCell
directly #17282 #17896).We need to find a better way to implement these components so they work anywhere a developer might insert them. And then, figure out how to style them properly when used in a bottom sheet.
Panels, what are they?
The README for Panel has this to say:
However, we don’t collapse the settings sections in mobile. Maybe we’ll want to do that at some point in the future, when we have more settings available, but it seemed like a better UX to not do that.
But we still use
Panel
, even though it’s function is to provide a title and spacing so a group of cells looks like a “section. Then we wanted some buttons in the settings outside any group, so we ended up withPanelActions
in #17703, even though those don’t have anything to do with the original meaning ofPanel
.Semantics vs. Looks
I think both problems reflect the same issue. Gutenberg has created several abstractions that have worked generally well. But, understandably, many of those have ended up tied to how things would end up looking in the web. It didn’t make sense to add an extra level of abstraction if things were only meant to be displayed in one way, but now things are different.
I think we have several problems to solve:
InspectorControls
. AInspectorControlsGroup
would allow a better abstraction where the web can show collapsible panels and the apps can have regular sections without collapsing. If we needed aButton
, we could just add one outside any group, and it would display as expected.Button
inside aInspectorControlsGroup
and have it automatically get the right style (without needing anInspectorControlsButton
). This has been done before on the web via CSS, but if we are exploring styled components and cross-platform solutions, I would like to solve this as well.BottomSheet
is in the previous point. Being able to define howRangeControl
looks in aBottomSheet
might be enough, but we could need newer abstractions to make those work, and that’s OK/cc @WordPress/native-mobile