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

Styled System exploration for Cross Platform components #1386

Closed
pinarol opened this issue Sep 25, 2019 · 10 comments
Closed

Styled System exploration for Cross Platform components #1386

pinarol opened this issue Sep 25, 2019 · 10 comments
Assignees

Comments

@pinarol
Copy link
Contributor

pinarol commented Sep 25, 2019

Let's explore styled-system to see how it can be useful for designing cross platform primitives.

For this exploration we can use a CSS-in-JS library such as Styled Components. Although it is optional and if there's a better one feel free to use that one.

What should be done?

  • Introduce a Box component to replace div/View.
  • Layout and styling of the Box component should be possible via props. We shouldn't pass className to it, instead all the CSS will be hidden inside Box.
  • Replace div usages in gutenberg/packages/block-library/src/gallery/gallery-image.js with Box keeping the same look.
  • Lastly remove unused CSS lines.

Here's a branch with some already done work, let's checkout a new branch from this branch and keep working based on this: WordPress/gutenberg@rnmobile/master...experimental/styled-system-poc the branch still uses old way of getting up the development environment so npm run dev will still work for now. If at some point we need to update it from master we should refer to the updated guidelines.

@pinarol
Copy link
Contributor Author

pinarol commented Sep 26, 2019

note: isCompact is supposed to set to true when Gallery has >= 7 columns (according to existing CSS)

@pinarol
Copy link
Contributor Author

pinarol commented Sep 26, 2019

Let's also try to see if we can make use of styled-system's theme specification to replace theme(primary) usages inside gutenberg/packages/block-library/src/gallery/inline-menu.scss.

Note: The real values for theme attributes reside in gutenberg/bin/packages/post-css-config.js currently.

@dratwas
Copy link
Contributor

dratwas commented Sep 27, 2019

Hey @pinarol I did this task here - https://github.com/WordPress/gutenberg/pull/17614/files
I left it as a draft but it is ready to review

@pinarol
Copy link
Contributor Author

pinarol commented Sep 27, 2019

Looking super cool thank you 🎉The only thing left seems like the handling of the z-index, currently I see that they are all kept in one place: gutenberg/assets/stylesheets/_z-index.scss, and the value 20 is actually coming from line: ".block-library-gallery-item__inline-menu": 20, normally that file provides z-index( $key ) function to get that value. We can propose a Javascript equivalent for that 🤔But no need for a big file, just a small alternative to the existing one, since we are just trying to demonstrate an idea here.

@dratwas
Copy link
Contributor

dratwas commented Sep 27, 2019

Z-index is also supported by theming. In this commit, I use z-index from theme: WordPress/gutenberg@52791f6

@pinarol
Copy link
Contributor Author

pinarol commented Sep 27, 2019

Super cool thank you! Could you open a PR then?

@pinarol
Copy link
Contributor Author

pinarol commented Sep 27, 2019

Thanks for the PR! 🎉 At this point, I have a question about the ThemeProvider. Would that work on mobile too?

Another thing is, I think we need to demonstrate a mobile usage as well, could you be able to replace some Views for this purpose? video block can be a good case:
gutenberg/packages/block-library/src/video/edit.native.js
It'd be good if we can open a separate PR for mobile side.

@dratwas
Copy link
Contributor

dratwas commented Sep 30, 2019

@pinarol ThemeProvider works for mobile since it uses React Context API :)

Will create PR for the video block today :)

@pinarol
Copy link
Contributor Author

pinarol commented Sep 30, 2019

@pinarol ThemeProvider works for mobile since it uses React Context API :)

@dratwas thank you for making that more clear!

Will create PR for the video block today :)

🙇

@pinarol
Copy link
Contributor Author

pinarol commented Oct 17, 2019

I am closing this for now since the prototypes are completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants