-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Add Card Component #17963
Conversation
This update enables JS to use the color hex codes defined in Sass. The hex codes are copied and stored as constants (objects). The key/value pairs are accessed through a convenient custom getter function.
This update adds the new Card component with it's various sub-components. A set of stories (Storybook) were added for the Card and it's sub-components for UI development.
This update refactors the import/export method of the Card components to follow current conventions, which allows for better code splitting and tree shaking. Unit tests were added for the Card components, as well as tests for how the sub-components integrate with the primary Card.
And fix border style rendering for Header and Footer
@@ -63,14 +69,28 @@ export { default as ToolbarButton } from './toolbar-button'; | |||
export { default as Tooltip } from './tooltip'; | |||
export { default as TreeSelect } from './tree-select'; | |||
export { default as IsolatedEventContainer } from './isolated-event-container'; | |||
export { createSlotFill, Slot, Fill, Provider as SlotFillProvider } from './slot-fill'; | |||
export { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm! The following changes were from my editor's auto format, which leverages ESLint + Prettier from the project's config.
I can revert this if necessary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please. All contributors use their own tooling and workflows, if they would use other formatting rules, we would see the same lines converted back and forth. In addition, this only increases the number of lines you have to scan during the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at the Storybook and noticed that fonts are not really defined or enforced and I was wondering about the guidelines and how do other design systems address fonts? Do we enforce them in components, or do we rely on a global style? If it's the latter, should we add such global style to Storybook?
About the Card component, since it's a base component, I'd love to see it used in Gutenberg as well. Do you think we can replace the Panel
s used in the sidebar for instance, or maybe find another place to use it?
packages/components/package.json
Outdated
@@ -43,6 +44,7 @@ | |||
"react-dates": "^17.1.1", | |||
"react-spring": "^8.0.20", | |||
"rememo": "^3.0.0", | |||
"styled-components": "^4.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this has any noticeable impact on bundle size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not tiny, but it's not terribly large either (e.g. moment.js)
It's currently at:
45.1kb, minified
16.4kb, minified + gzipped
https://bundlephobia.com/result?p=styled-components@4.4.0
The team is reducing the library size by a lot (and making it faster) in v5:
https://medium.com/styled-components/announcing-styled-components-v5-beast-mode-389747abd987
One of the requirements is that you must be using React 16.8, for hook support. Thankfully, it looks like Gutenberg + @wordpress/components
does :)
export const CardContext = createContext( {} ); | ||
export const useCardContext = () => useContext( CardContext ); | ||
|
||
export const CardProvider = ( props ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What value does this component add over just using the CardContext.Provider
one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question!
This was a pattern I started using as the Provider
became more complex. It was an abstraction to keep the initial entry component (the index.js
) code relatively simple. Sorta like how you might have a simple app.js
, but a separated complicated Redux action/reducer setup.
Since the current implementation + feature-set is very minimal, I can consolidate the Provider logic into the main index.js
file :). No need to pre-optimize - also, the code is clearer 👍
/** | ||
* External dependencies | ||
*/ | ||
import { shallow } from 'enzyme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's not written anywhere, but we've been trying to slowly move away from "enzyme". I think we settled on just using the tools provided by the React team to avoid having tests blocking us from upgrade React in future updates...
cc @gziolo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh boy! @gziolo , I didn't know about this thread! But I went over (slash ranted) about testing difficulty with Enzyme on my live coding session 😂 .
@youknowriad Is there an example of component tests I should follow?
I'm more than happy to refactor/remove the tests if it's inline with the direction and future of @wordpress/component
testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of update
to refresh components is very tricky and it was introduced as a workaround to fix their previous architectural decisions :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an example of component tests I should follow?
I'm more than happy to refactor/remove the tests if it's inline with the direction and future of@wordpress/component
testing
We probably don't have too many unit tests covering UI components, which use something different than Enzyme
. I guess it's fine to keep your tests for the time being and figure out our strategy in the linked issue (#17249).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Thank you!
In the parent issue they discuss the block directory feature as a good place to redo it using Card: |
First, love this! What I love about it is this both solves a component needed and the open way you went about creating it. Also, props on a really thorough PR - I love how detailed it is. From a design perspective this feels as expected, which is great. I can see how it could adapt and be customised to most situations. The minimal nature makes sense because it can iterate. @youknowriad's comment about the panel did get me thinking about where a panel and card differ and even if they should. This feels like a great conversation point. I can see how a panel is a card used in a particular context. |
I really like this too @ItsJonQ! Great work. Regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR got enough attention and it was reviewed and tested by several people. Let's call it an experiment and get reevaluate this approach in a few weeks whether CSS in JS works well for the project.
The immediate follow-up PR should try to use those newly introduced components in the existing codenase as discussed in the comments in this PR and its parent issue.
@@ -0,0 +1,4 @@ | |||
{ | |||
"presets": [ "@wordpress/babel-preset-default" ], | |||
"plugins": [ "babel-plugin-emotion" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was moved to storybook/.babelrc
- this one should be removed and the other one updated. It's a merge issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! Thank you for spotting
Removed the local babelrc from components. Added babel-emotion to global storybook/.babelrc
storybook/.babelrc
Outdated
@@ -1,6 +1,7 @@ | |||
{ | |||
"presets": [ "@wordpress/babel-preset-default" ], | |||
"plugins": [ | |||
"babel-plugin-emotion", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: whitespaces, I don't think we lint this type of files :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! My bad. Fixing now. Thank you for catching <3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, let's 🚢 when Travis is green
Thank you everyone involved to make this happen! 🎉
What has happened to Travis, why it didn't start for the last commit? 🤔 The previous one https://travis-ci.com/WordPress/gutenberg/builds/136300101 looks good. |
@gziolo Not sure~! Just restarted it 😊 |
Nice work everyone 👏 |
@@ -35,6 +37,7 @@ | |||
"classnames": "^2.2.5", | |||
"clipboard": "^2.0.1", | |||
"dom-scroll-into-view": "^1.2.1", | |||
"hex-rgb": "^4.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hex-rgb
uses modern JavaScript (const
, let
, etc.) and is not transpiled to be able to run in older browsers.
tl;dr: Including this library has broken the editor in Internet Explorer.
Apparently this is an intentional choice on their part: sindresorhus/hex-rgb#10 (comment)
This module mainly targets Node.js, not the browser. It's up to you to transpile it with Babel if you want to use it in the browser.
If it's something we plan to use, it would need to be transpiled as well. I don't recall having had to deal with this for any other dependencies, and it would probably require some work to adapt into our build tooling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Long term I think its probably good idea to support this in our tooling just as CRA, Parcel and other tools in the JavaScript ecosystem have done. Switching to polished
's parseToRgb
or similar fn might be a quick fix.
Edit: I'm working on the quick fix now
Edit: The quick fix is here #18681
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, thinking about the longer-term fix, I saw this on Twitter the other day and it seems that not compiling (or only compiling so-far) is closer to becoming a defacto standard (yay for smaller bundle sizes for apps only supporting modern browsers!).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR for updating the build tooling #18798
Wondering how you got emotion to work with wp and gutenberg blocks? i am having trouble with server side rendering when i refresh |
@gracelauren Hiya! Apologies for the late reply! It looks like the convo was moved here: |
Description
This update adds a new
<Card />
component to@wordpress/components
, as well as a collection of flexible sub-components that can be used to composed various user interfaces. The initial sub-components include:<CardBody />
<CardDivider />
<CardFooter />
<CardMedia />
🚀 Deploy Preview
https://deploy-preview-17963--gutenberg-playground.netlify.com/design-system/components/?path=/story/card--default
✨ Visual Design
The
<Card />
component's designs are very minimal. Theborder-radius
,border-color
, andpadding
are based on the aesthetics and values found within@wordpress/components
and Gutenberg.🤔 Component API Design
<Card />
drew it's component API (and it's composition) from other frameworks/libraries like Bootstrap and Material UI.An example for how a
<Card />
UI my be composed looks something like this:How has this been developed & tested?
📚 Storybook
<Card />
was developed entirely in Storybook. The Knobs add-on was added to make developing and testing easier. I've set up knobs for various props and configurations to see how the various sub-components can fit together.Knobs was incredibly valuable in the development of
<Card />
. It helped me improve the iteration cycle/testing of Context (which the component uses internally). The Storybook UI allowed me to catch and fix breaking edge-case CSS issues.Lastly, Storybook was very helpful in fine-tuning the
<CardMedia />
component, which relies more on responsive behaviour:🧪 Jest / Enzyme
I've written unit tests for all of the various sub-components. I've also added integration tests for sub-components ->
<Card />
. The naming conventions and testing techniques were drawn from the existing Enzyme based tests I found within@wordpress/components
. I didn't add anything new 😄 (as far as architecture, tech, etc... goes)Types of changes
💅 Styled Components
This is new and experimental! For the
<Card />
styles (and it's sub-components), I introduced and used styled-components to take care of the CSS.I've seen a couple of PRs attempting this as well:
#17673
#17614
The benefits of a CSS-in-JS solution are many. Those PRs do a fantastic job at walking through several key benefits - especially for native mobile.
I think it's best feature is... components will be styled and working out-of-the-box. If you build a React component with styled components, you can
import
the component perfectly styled straight fromnode_modules
. No fussing with Webpack, Grunt, compilers or scripts. It provides for a wonderful developer experience. Not only that, styled components will provide features like theming. This will make it much easier for folks to consume@wordpress/components
to build their own interfaces - for their own blocks, plugins, or interfaces outside of Gutenberg.An incredible example of this experience can be found in the Material UI React library. All of the components come pre-styled and can be themed (Note: They use an alternate CSS-in-JS library, not styled components).
🌲 Preserving standard CSS styling
This is 💯 important. To do this, I added a standard CSS className to the various components (e.g.
components-card-header
), just like the rest of@wordpress/components
. Although these classNames aren't responsible for direct styling, they enable:html
composition when debugging (e.g. Inspect Element)🎨 Color Utils
Since I ventured into CSS-in-JS for the pull request, I needed a way to use the same colour values we have from
.scss
but for.js
code. To do this, I built acolor()
utility, which lives within the newcomponents/utils
directory.It can be used like this:
The
.
notation represents the nested colour values I have stored in the constantcolor.values.js
map (Object
).💪 Testing it all
I (temporarily) updated the
block-library/button
component with the new<Card />
component - wrapping it's contents. In addition to working with Storybook and Jest, I needed to make sure the code worked with Gutenberg (of course 😂).In this test, I was able to see that the Styled Components render components worked with the existing components and styles (and the thing didn't explode).
📹 Live Coding
I coded these components via YouTube livestream this past Friday (and a bit this morning):
Here are the videos, in case anyone is interested:
https://www.youtube.com/watch?v=oAPuOB9oYCw
https://www.youtube.com/watch?v=Kf-CtY3wQGM
My thinking for the live coding was to demonstrate and explain my thinking throughout the design process. Also, it was wonderful to engage with folks on chat. There were several points where we collaborated to figuring out specific component API and behaviour. It was lovely 💕.
🤗 Feedback
I would love to know what you folks think! I understand that some of these approaches are new in the context of
@wordpress/components
and Gutenberg, specifically Stories and Styled Components. I'm more than happy to answer questions!Thank you so much for your time 🙏
Checklist:
Potentially addresses:
#15921