Introduce a common API to style Button from both web and mobile by dratwas · Pull Request #19104 · WordPress/gutenberg

dratwas · 2019-12-12T14:35:59Z

Description

This is an exploration of x-platform style system. wordpress-mobile/gutenberg-mobile#1411
This is the second iteration on wordpress-mobile/gutenberg-mobile#1386

Mobile side PR: #19179

In this PR I used styled-system with emotion to create a primitive Button component that could be used in web and mobile and could be styled via props.

I added A and button components since both of them are used in @components/Button. They accept className as a prop to be backward compatibility ( we need to support already existing themes ) and all style props that are supported by styled-system
I added a PrimitiveButton that is described here - Introduce a common API to style Button from both web and mobile #19104 (comment)
I added the theme provider and theme values (they could be a bit outdated since I took them from my previous PR [Experimental] Cross platform Box component to replace div #17614 )
I used created primitives in @components/Button (after that all buttons uses the primitives)
I passed styles via props instead of CSS in the Inserter button.
In addition, I created BlockBreadcrumbButton components that use PrimitiveButton instead of @components/Button just to demonstrate.

How has this been tested?

Run Gutenberg web
All buttons should look the same as before
Check if BlockBreadcrumbButton looks correct with and without hover/focus
Check if Inserter button looks correct
The preview button should be different than originally - it should be 50px height and has bigger borderRadius.

Screenshots

The BreadcrumbButton component that uses PrimitiveButton with additionalStyles prop to add hover, focused styles - https://github.com/WordPress/gutenberg/pull/19104/files#diff-21bd8baf92167b81915be200a8dd9eb8

:hover

:hover:focus

:hover:focus:active

The Inserter button that uses @components/Button with some changed styles by additionalStyles and styled-system props - https://github.com/WordPress/gutenberg/pull/19104/files#diff-c6139f159c9b2b79af303587407aea51

:hover

:hover:focus

:hover:focus:active

Types of changes

Experimenting with emotion and styled-system

PrimitiveButton checklist

States
States change
- on the web, we can use onFocus, onBlur to determine the focus state. MouseEvents to determine the active and hover state. We specify whether the button is enabled or disabled by prop so we don't need to listen to that change. The only state left is visited but I'm not sure if we can get that information somehow and also if the visited is also applicable to button or only to the a element.
Icons & Labels
Classname
Visibility
CSS prop

components/Button checklist

Components/Button should have the same props PrimitiveButton has + variants
Styles passed via props will override variant styles
- They do but i.e even if i use marginLeft in the inserter button, this is not applied because of styles that are declared in a different place and are more specific (i think it should work like that because of themes that can override styles) but they override styles that are declared in components/Button
keep the backward compatibility

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

ItsJonQ · 2019-12-12T17:37:49Z

@dratwas I know you just started this. Just wanted to say that I'm really excited to see this happening :D

packages/components/src/styled-primitives/button/index.js

dratwas · 2020-01-31T15:32:13Z

There are notes made by @pinarol during our conversation about that PR. We will add the PrimitiveButton component.

PrimitiveButton

States

We’ll have a defined set of states:

focus
active
enabled
hover
disabled
visited

Each should be documented for each platform, not every one of them has to be available for both mobile and web but they should be limited to what has defined.

We should:

Allow style customizations for each state
Allow combining the states

This can be similar to:

additionalStyles={[{ states: 'focus:hovered:visited', styles: {...} }]}

State changes

We’ll provide event handlers to state changes if possible. Needs investigation.

Icons & Labels

Icons or labels won’t be part of PrimitiveButton API, they will be passed as children:

<PrimitiveButton hoveredStyle bg="primary" heigh={50} >
  <Text>Some text </Text>
  <Icon />
</PrimitiveButton>

classname

We should have the classname prop to allow the style overrides by themes.

::after ::before
It looks like we may need to support these in the API for web because we don’t have a way for getting informed by state changes. But this needs more investigation.

Visibility

This component will be public as it has some use cases in some plugins: https://github.com/Automattic/jetpack/blob/master/_inc/client/components/button/style.scss

CSS prop

There won’t be any css prop since it is violating x-platform usage.

components/Button

We don’t necessarily need to use the PrimitiveButton in it we can use emotion’s button
Components/Button should have the same props PrimitiveButton has + variants
Styles passed via props will override variant styles

<Button link marginLeft={5} height={50} >
  <Text>Some text </Text>
</Button>

...will render a link button with 2 difference only: marginLeft={5} height={50}

We’ll keep the backward compatibility

youknowriad · 2020-02-26T11:43:25Z

packages/block-editor/src/components/block-breadcrumb/breadcrumb-button.js

+
+export default function BlockBreadcrumbButton( { children, ...props } ) {
+	const theme = useTheme();
+	const additionalStyles = [


Is this for things that don't translate to mobile?

Since we don't want to expose the css prop in public API, there is no way to set styles for i.e hover, focus states. Our first idea was to use props like hoverStyle, activeStyle ... but these styles can be mixed so we will end with hoverFocusActiveEnabledStyle and that would be hard to use. So we decided to add additionalStyles prop where we can set styles for different states.

So these work for mobile too?

The short answer is “yes”, but not all of them. For example “hover” is invalid for mobile as there’s no cursor. We should have done a better job documenting these states. I’ll add it to this PR.

youknowriad · 2020-02-26T11:44:43Z

packages/block-editor/src/components/inserter/index.js

+			border={ 'none' }
+			outline={ 'none' }
+			padding={ 'medium' }
+			margin={ 0 }


I get that we want these props for a primitive but I don't feel very good having these leak to a higher-level abstraction (UI components) I'd rather have variations in the component itself.

I think we can really use your and other reviewers’ guidance at this point about how to make this better. We also had concerns about overwriting styles for variants like isPrimary, isSecondary, however, we found in many places that Button style is overridden in CSS in addition to using variants, like here or here. The idea behind this is, being able to meet the same need in an x-platform way. Otherwise, those kinds of customizations will not work on mobile. We could refactor these Buttons that have overridden styles to use PrimitiveButton instead but it will require a lot of extra work. I really like what @pinarol suggested to put all styling params like margin, padding (not sure about additionalStyles) etc. to one and call it experimentalStyles, in that way we will expose only one prop in public API, WDYT about it?

however, we found in many places that Button style is overridden in CSS in addition to using variants, like here or here.

For me instead of doing that, I prefer if we should be looking at why we did these overrides in the first place and avoid them entirely with better abstractions/components/variations. These overrides quickly lead to bugs anyway.

This was something we were questioning as well, “should we allow overriding the styles already defined in variants”? The ideal answer to this is actually “no”, this is also how syled-system variants page recommends: https://styled-system.com/variants given that you are sharing a similar opinion I think we can remove the props that are already defined by variants. However, we’ll still need to provide some props to be able to position the Button in a x-platform way like here or here. Positioning/layout related props need to be available among all x-platform components not just Button, and those are: space, layout, flexbox, position from https://styled-system.com/api Otherwise we won’t be able to provide an x-platform component, wdyt?

I'm not certain I understand exacly why we need the positionning props absolutely. There are alternatives, Layout components (flex containers aware of their children,toolbars...). I'm not saying we won't ever need these but I believe it's better to start with no CSS props at all and consider use-case per use-case if there are better alternatives.

There are alternatives, Layout components (flex containers aware of their children,toolbars...).

Agree, but sometimes it's not enough and the 'margin', 'width' etc are needed. At this moment they are set in CSS like here but we are not able to do the same on mobile. So we would need to wrap the button into some container (View) and add the margins to the container which is not performant and adds one more nesting level.

I'm not saying we won't ever need these but I believe it's better to start with no CSS props at all and consider use-case per use-case if there are better alternatives.

That's actually a good idea to do less in that PR. We could get rid of these props and then add them if needed during refactoring. However, I still feel like adding the spacing props will be one of the first things that will happen. There is a small part of the list of components that overrides these styles:

gutenberg/packages/components/src/date-time/style.scss

Line 37 in 4dd6a8c

.components-button {

gutenberg/packages/components/src/dropdown-menu/style.scss

Line 59 in 4dd6a8c

min-height: $icon-button-size;

gutenberg/packages/components/src/guide/style.scss

Line 7 in 4dd6a8c

align-self: flex-start;

gutenberg/packages/components/src/guide/style.scss

Line 61 in 4dd6a8c

height: 30px;

gutenberg/packages/components/src/notice/style.scss

Line 40 in 4dd6a8c

margin-left: 4px;

gutenberg/packages/components/src/notice/style.scss

Line 80 in 4dd6a8c

margin-top: -2px;

gutenberg/packages/components/src/placeholder/style.scss

Line 93 in 4dd6a8c

margin-right: $grid-unit-15;

gutenberg/packages/components/src/placeholder/style.scss

Line 107 in 4dd6a8c

margin-left: 10px; // equal to standard button inner padding

gutenberg/packages/components/src/toolbar-button/style.scss

Line 2 in 4dd6a8c

position: relative;

I also checked for example the rebassjs lib that is UI component library and they allow to set i.e margin to button that is primary https://rebassjs.org/button/

Actually, we referred to the already existing css customizations for Button component while adding the new style props. However I am now discovering that we actually don't quite approve most of those customizations either. What I am confused about is, isn't this a good reason to lean towards to a more encapsulated API design?

If we hide the css inside the component and manage styles via props we’ll have a possibility to deprecate/remove those which we don't want to be overridden.

Currently we are discussing what to “expose” in the API and I think it is a very valuable discussion to have. But doesn’t current state already expose every overriding possibility by providing the className publicly in the API? Also as far as I see we are already tied to a specific class name(components-button), just to provide overriding capabilities I assume?

If we look at it this way this PR actually isn’t extending what is already exposed. On the contrary I think it can be a first step to be in control of what to expose in the future.

Having said that, I think it’d be good to group style related props under one experimental prop at this stage, that would make us more free about trying different possibilities.

About having margins/width/height in the API: I am thinking what can be the alternative approach?

Designing the Button in such a way that always fills the container?

This requires putting a container per every Button just to be able to set margins/width/height, thus, doesn’t feel right.

Determining margins/width/height inside the implementation, so nobody would be able to override?

And this doesn’t look like a realistic goal considering almost every usage of Button sets a different margin. Plus, I believe it’s a fair need to change the margins/width/height of the component.

youknowriad · 2020-02-26T11:46:04Z

packages/components/src/index.js

 	G,
 	HorizontalRule,
 	BlockQuotation,
+	PrimitiveButton,


I left these exports here only for Backward compatibility, ideally we should just consume the primitives from the primitives package.

Let's not expose other primitives here.

youknowriad · 2020-02-26T11:46:44Z

packages/components/src/index.js

 export { default as withNotices } from './higher-order/with-notices';
 export { default as withSpokenMessages } from './higher-order/with-spoken-messages';
 export * from './text';
+export { useTheme, default as ThemeProvider } from './theme-provider';


This probably needs more time to mature as it's the first CSS in JS public API, can we start with experimental APIs?

youknowriad · 2020-02-26T11:48:25Z

packages/components/src/theme-provider/index.js

+
+export default ( { theme, children } ) => (
+	<ThemeProvider theme={ getTheme( theme ) }>{ children }</ThemeProvider>
+);


Since this introduces theming, I'd like to know more (and maybe even document) how do we build themable components, how are we planning to update our UI components to be themable and how to use these APIs in general.

youknowriad

Nice work here.

Let's give this PR more visibility in the next #core-js #core-css chats and among the @WordPress/gutenberg-core team as it can be impactful.

gziolo · 2020-11-28T07:13:20Z

What’s the status of this proposal? There is now many components redactored in @wordpress/componnents to use CSS in JS approach so it might be more relevant than ever. @hypest and @Tug, any thoughts?

dratwas · 2021-02-10T18:21:38Z

Hey @gziolo, unfortunately, I do not know the status of this proposal since we were waiting for the decision from gutenberg/core team. I started doing other tasks and we never got answers to our questions ;(

I believe it is still valid even though it was a year ago. What I can suggest is to start from resolving conflicts then revisit all button styles (web and mobile) as I did previously and add new variants if any.

There is also an important discussion about props that can be passed to the components: #19104 (comment)

hypest · 2021-02-11T09:26:12Z

cc @chipsnyder

gziolo · 2021-02-12T08:16:31Z

@dratwas, thank you for the update. It looks like this topic didn't get enough attention.

@ItsJonQ is leading efforts to create a new component system designed with the goal to become the primary way to build UIs. He is working on an official proposal to have it included in the WordPress core. At the moment, it's web-only, but it uses the architecture which should make it possible to integrate it with mobile in a similar way as here.

@ItsJonQ and @sarayourfriend should move the code that abstracts styles to the repository quite soon. All the code for the component system is located in the following folder packages/components/src/ui/. It's still in development, so there is no final public API defined. I'm sure @ItsJonQ has good opinions on how it should be shaped but if you would discover any blockers from the mobile side, let us know.

ItsJonQ · 2021-02-16T14:10:07Z

Thanks for the mention @gziolo !

For added context, the Style system for the new component system can be found here:
https://github.com/ItsJonQ/g2/tree/main/packages/create-styles

As @gziolo mentioned, it will be moved over to @wordpress/gutenberg as part of our integration.

Under the hood, it's using Emotion (v10), just like our current solution.
However, there are a lot of crucial enhancements, which are currently web specific - notably how variables work.
That being said.. A lot of how styles are created/accessed are done through functions that we have control over (e.g. styled, css(), or get() (for variables)). To add React Native support, we can adjust the behaviour of these functions specific for the RN platform (in theory).

It's not something I've explored yet.

My personal RN experience has been limited to the Expo development experience.
For my app project, I used emotion for styling, so I have some RN x Emotion experience in that sense.

dratwas added 2 commits December 12, 2019 15:03

Add button primitive and use it in block breadcrumbs component

67f2a24

fix import

4f68460

Use primitive button for block breadcrumb

d9b7907

dratwas force-pushed the experimental/button/styled-system/emotion branch from f8b4bf3 to d9b7907 Compare December 16, 2019 17:15

dratwas changed the title ~~[Experimental] Add button primitive~~ [Experimental][emotion] Add button primitive Dec 16, 2019

dratwas added 2 commits December 17, 2019 13:53

use for inserter

cc50ba5

fix styling

851e8dc

dratwas marked this pull request as ready for review December 18, 2019 09:49

dratwas requested review from ajitbohra, chrisvanpatten, ellatrix, jorgefilipecosta, talldan and youknowriad as code owners December 18, 2019 09:49

dratwas added 4 commits January 7, 2020 15:12

Merge branch 'master' into experimental/button/styled-system/emotion

005edbf

Merge branch 'master' into experimental/button/styled-system/emotion

a4bbb77

fix tag props

45089b9

WIP

818a7ba

pinarol reviewed Jan 15, 2020

View reviewed changes

packages/components/src/styled-primitives/button/index.js Outdated Show resolved Hide resolved

dratwas added 4 commits January 16, 2020 16:21

Move rest of styles and add helper function for shade/tint color

1c81160

add hover, focused, disabled

6f3b517

add possibility to override pseudo-classes of variants

beddc82

WIP split styles

27ab231

dratwas added 5 commits February 5, 2020 18:46

add PrimitiveButton and additional style util

3d8c221

use primitive button in breadcrumbs

53b2483

Refactor styles of components/Button and add additionalStyle prop

f36f2c2

remove todo

b5025d4

remove unused styles

898c456

pinarol requested a review from gziolo February 26, 2020 10:47

pinarol added Needs Decision Needs a decision to be actionable or relevant Needs Technical Feedback Needs testing from a developer perspective. Needs Testing Needs further testing to be confirmed. labels Feb 26, 2020

youknowriad reviewed Feb 26, 2020

View reviewed changes

dratwas added 5 commits February 27, 2020 17:34

change name of experimental things

47d612d

add readme to the primitive button

71d740c

remove TODO from readme

07c2976

fix code styling

1afa13c

Fix type of additionalStyles and add inline docs to helper

f1fe798

pinarol mentioned this pull request Mar 10, 2020

[RNMobile] - Gallery Block: Columns Control #20231

Merged

6 tasks

koke removed their request for review April 2, 2020 15:30

dratwas added 4 commits April 3, 2020 15:58

Merge branch 'master' into experimental/button/styled-system/emotion

63aaf31

fix build issues

e4b631f

remove theme provider

d14b935

remove theme provider and apply new styles

98d643a

gziolo added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 28, 2020

Base automatically changed from master to trunk March 1, 2021 15:42

dratwas closed this Jul 10, 2023

gziolo deleted the experimental/button/styled-system/emotion branch July 10, 2023 12:40

Conversation

dratwas commented Dec 12, 2019 • edited by pinarol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

:hover

:hover:focus

:hover:focus:active

:hover

:hover:focus

:hover:focus:active

Types of changes

PrimitiveButton checklist

components/Button checklist

Checklist:

Uh oh!

ItsJonQ commented Dec 12, 2019

Uh oh!

Uh oh!

dratwas commented Jan 31, 2020 • edited by pinarol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PrimitiveButton

States

State changes

Icons & Labels

classname

Visibility

CSS prop

components/Button

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dratwas Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dratwas Feb 27, 2020 • edited by pinarol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dratwas Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo commented Nov 28, 2020

Uh oh!

dratwas commented Feb 10, 2021 • edited by gziolo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypest commented Feb 11, 2021

Uh oh!

gziolo commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dratwas commented Dec 12, 2019 •

edited by pinarol

Loading

dratwas commented Jan 31, 2020 •

edited by pinarol

Loading

dratwas Feb 27, 2020 •

edited

Loading

dratwas Feb 27, 2020 •

edited by pinarol

Loading

dratwas Feb 27, 2020 •

edited

Loading

dratwas commented Feb 10, 2021 •

edited by gziolo

Loading

gziolo commented Feb 12, 2021 •

edited

Loading