Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Experimental] Cross platform Box component to replace div #17614

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Sep 26, 2019

Description

As the Gutenberg mobile team, we've been looking for ways to build a cross-platform components library which can work on both web/mobile. We want to reach to a point where we don't need to implement extra code to add mobile blocks. This requires us to find a way to have a common styling system between web&mobile. Since CSS is not supported by React Native we started to investigate styled-system and styled-components for this purpose.

We are planning to use this only for the editor, thus we won't be touching save.js files. So the result html we see on sites won't get effected. Please keep this in mind while evaluating its side effects on existing themes/sites. We want to find out how much CSS and related class names are essential for theming and we want to understand what we might be breaking.

Here are some useful links related to these technologies:

styled-system:
https://styled-system.com
https://styled-system.com/how-it-works

styled-components:
https://www.styled-components.com/
https://www.styled-components.com/docs/faqs

In this PR we are only focusing on web part but we have another PR which demonstrates how Box component can be used on mobile as well: #17673

Summary about used frameworks

styled-system
styled-system is a collection of utility functions that add style props to your React components and allows you to control styles based on a global theme object with typographic scales, colors, and layout properties.

styled-components
styled-components is a CSS-in-JS library which can convert the props of your component into real CSS. It is using tagged template literals (a recent addition to JavaScript) to evaluate styled.div...,
and it allows you to write actual CSS code to style your components, but the css is kept inside the bounds of a component. So it allows you to combine it with the already existing CSS, check this out: https://www.styled-components.com/docs/faqs#can-i-use-css-frameworks

Although we chose styled-components, I'd like to mention that, styled-system can also be used with another CSS-in-JS library. So we are not tightly bound to styled-components.

Please refer to this comment to see what kind of html it produces: #17614 (comment)

We are trying to be careful about not effecting the existing themes but we might be missing something. So please go ahead and have a look at this PR and let us know if this introduces a breaking change on your site.

Related issue - wordpress-mobile/gutenberg-mobile#1386

How has this been tested?

Open gutenberg on the web, create a new gallery or select an existing one and check if move and delete buttons look and work as expected

Screenshots

Screenshot 2019-09-27 at 11 28 10

Types of changes

Experimental

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.

@dratwas dratwas marked this pull request as ready for review September 27, 2019 12:58
@pinarol pinarol changed the title [Experimental] styled-system + styled components [Experimental] Cross platform Box component to replace div using styled-system + styled components Sep 27, 2019
@pinarol pinarol changed the title [Experimental] Cross platform Box component to replace div using styled-system + styled components [Experimental] Cross platform Box component to replace div Sep 27, 2019
@pinarol
Copy link
Contributor

pinarol commented Sep 27, 2019

Hey @youknowriad @gziolo 👋 Checkout this new proposal about replacing 'div's with their x-platform equivalents and please feel free to add other reviewers who might be interested. Looking fwd to hear from you!

@pinarol pinarol requested a review from koke September 27, 2019 14:28
@pinarol pinarol requested a review from Tug September 27, 2019 14:34
@gziolo
Copy link
Member

gziolo commented Sep 28, 2019

I’m sharing the link with the article how all this work:
https://medium.com/styled-components/how-styled-components-works-618a69970421

If you know better resources please include as I spent some time googling and this is the only relevant article I could identify.

@pinarol
Copy link
Contributor

pinarol commented Sep 29, 2019

@gziolo Sure! Sorry for not sharing before.

Styled-system web site:
https://styled-system.com
https://styled-system.com/how-it-works

Styled-components:
https://www.styled-components.com/
https://www.styled-components.com/docs/basics

Styled-system and styled-components frameworks are not tightly coupled so one can use styled-system with another ‘CSS in Javascript’ solution.

This talk can also help to gain a basic idea about how it works:

Lastly there’s a Babel plugin(not used in this PR) which brings some useful enhancements that we might want to use in case we choose to proceed with this way. https://www.styled-components.com/docs/tooling#babel-plugin

Hope this helps

@gziolo
Copy link
Member

gziolo commented Sep 30, 2019

@pinarol, I didn’t watch the video shared but otherwise the linked resources explain how to use it rather than what happens for the web. The biggest question is whether you can set it up in a way where you could generate stable class names so the changes introduced wouldn’t have impact on all websites which override styles with CSS.

import { color, space, layout, flexbox, background, border, position, shadow }
from 'styled-system';

const Box = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this automatically translate to View on native or would we need a separate implementation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we'll need a sparate box/index.native.js for that. Planning to open a separate PR for mobile

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was hoping styled-components would bring more reusability there. I imagine the implementation of that would be exactly the same except using styled.View instead of styled.div?

How would we implement other components, like the one to replace the <figure> in GalleryImage? Would that also be the same as this but a styled.figure?

Borrowing from the previous HTMLElement experiment, I would expect Box to be a more generic container thing that would accept a tagName prop to be used on the web implementation.

I think the syntax of styled-components would allow for this, but I'm not sure what that looks like yet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine the implementation of that would be exactly the same except using styled.View instead of styled.div?

Right, that's also an option. We can do a platform check and call styled.View if it is mobile.

How would we implement other components, like the one to replace the

in GalleryImage? Would that also be the same as this but a styled.figure?

yes, you can either use a valid react/react-native component or a tagname like div, figure, h1 etc.

@pinarol
Copy link
Contributor

pinarol commented Sep 30, 2019

@gziolo I think your question is addressed here: https://www.styled-components.com/docs/advanced#existing-css

I guess the short answer is: Yes we can pass stable classnames to be used with generated ones, but custom css owners might need to bump up the specificity of some selectors to take precedence.

@pinarol
Copy link
Contributor

pinarol commented Sep 30, 2019

@dratwas could you update the PR to demonstrate passing stable classname(the old one in this case)?

@gziolo
Copy link
Member

gziolo commented Sep 30, 2019

I guess the short answer is: Yes we can pass stable classnames to be used with generated ones, but custom css owners might need to bump up the specificity of some selectors to take precedence.

To make it clear. The general idea of WordPress is that you don't use auto-generated class names, but all of them have predictable names and can be overridden by theme developers. This is why I'm trying to understand how close we can potentially get to the current state of the art. It mostly required to have a full picture of what is really on the table if we would decide to bet on the styled system. It's rather a huge shift in the direction of the project so we need to know all the implications it creates.

@pinarol
Copy link
Contributor

pinarol commented Sep 30, 2019

@gziolo Can I learn more about what kind of procedure we are following right now if we need to update any inner CSS? As far as I understand any change has always a possibility to break themes?

@dratwas
Copy link
Contributor Author

dratwas commented Sep 30, 2019

Hey @pinarol, I use previous class names to change the background color of the box in my latest commit. As you can see I set the background color to #ffffff in the box component but it still has a blue background because of styles in scss file. af23aee

padding={ isCompact ? 0 : 'small' }
position="absolute"
zIndex={ 'block-library-gallery-item__inline-menu' }
bg={ isSelected ? '#ffffff' : 'inherit' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this line is just for demonstrating that it is possible to override this value with custom CSS from outer place. You'll see #ffffff is not getting effective since we pass block-library-gallery-item__move-menu and block-library-gallery-item__inline-menu to Box component.

@pinarol
Copy link
Contributor

pinarol commented Sep 30, 2019

Let me drop this image to explain how it works better:

Screen Shot 2019-09-30 at 20 57 57

Each node has minimum of 2 classes connected to it. We can always pass our own className as seen in the screenshot (in this case block-library-gallery-item__move-menu and block-library-gallery-item__inline-menu) and that will be added at the end. We haven't done it yet but we can additionally pass "is-selected" according to the selection status as well.

About the generated classnames:

1st one is the same for all components of same type and it is static. For example <Button /> would render with the same static class every time.
2nd one is unique per usage of that element, it is dynamic.
More info about this can be found here: https://www.styled-components.com/docs/faqs#why-do-my-dom-nodes-have-two-classes

We can continue having same className applied to the same component, so how does this sound considering external CSS?

I have another question regarding the CSS in themes. Is that a common thing to modify Gutenberg editor via themes? And is there a limit to it(like "we don't want themes modifying block picker")?

FAQ page can also be handy at this point as it answers some of our questions:
https://www.styled-components.com/docs/faqs#can-i-use-css-frameworks

@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Feature] UI Components Impacts or related to the UI component system Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Oct 1, 2019
@ItsJonQ
Copy link

ItsJonQ commented Oct 1, 2019

@dratwas 😍 Wowow! Thank you so much for putting this together! I'm a huge fan of CSS-in-JS solutions, especially the ones that work like styled-components/emotion.

💯 👍 for me.

...auto-generated class names

@gziolo Ah yes! As @pinarol demonstrated in their screenshot, the technique to compliment constant classNames would be to continue using them as normal. The auto-generated className from styled-components is added on.

If the user chooses to enhance or override styles (via the traditional CSS method), they can target the constant className (e.g. block-library-gallery-item__move-menu).

I'm a big believer in this method of handling classNames. By continuing to use traditional classNames, it:

  1. Allows for traditional CSS targeting
  2. Makes it easier to understand what you're looking at when you "Inspect Element" (especially for non-React devs)

Bonus: 2.5: Forces the component developer(s) to pause for a brief second to consider the DOM structure when they add new HTML elements.


Edit: @dratwas just for my knowledge, would the <Box /> component be used purely for layout/UI purposes? In other words... use it wherever you would normally use a div for UI things.

For everything use, like h1, used styled.h1 instead?

I'm asking because I haven't used styled-systems within a big project before 😊. I'm a fan though! (of that, and basically everything @jxnblk makes)

@pinarol
Copy link
Contributor

pinarol commented Oct 1, 2019

I've updated the PR description with some background information about why we are doing this. Also provided some more info about the utilized technologies.

@pinarol
Copy link
Contributor

pinarol commented Oct 1, 2019

would the component be used purely for layout/UI purposes? In other words... use it wherever you would normally use a div for UI things.

Right, it is cross-platform equivalent of div for web, and View for mobile. There's another PR that demonstrates it being used instead of View.

For everything use, like h1, used styled.h1 instead?

Yes that's the way it works, you can either use a valid react/react-native component or a tagname like div, figure, h1 etc. with styled function. But we should note that styled usage will be hidden inside the component implementation.

@hypest
Copy link
Contributor

hypest commented Feb 21, 2020

Is this PR still in progress? Can we close it with some conclusion if it has reached its end?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants