Skip to content

Update the padding values on the Card component to align with proposed spacing system#21111

Merged
ItsJonQ merged 6 commits intoWordPress:masterfrom
jameskoster:update/Card
Apr 7, 2020
Merged

Update the padding values on the Card component to align with proposed spacing system#21111
ItsJonQ merged 6 commits intoWordPress:masterfrom
jameskoster:update/Card

Conversation

@jameskoster
Copy link
Copy Markdown
Contributor

Description

This PR updates the padding values on the Card components (header, body, footer) to be multiples of 8, and align more neatly with the "consistent spacing system" proposed here. While multiples of 4 are totally acceptable, being a little stricter and using 8 as a preference creates a greater sense of consistency and robustness.

I also renamed the size prop to padding which is more explicit. Size felt a bit too ambiguous.

How has this been tested?

Testing required :)

Screenshots

padding

The top row are the current values, the bottom row are the proposed values.

Types of changes

CSS changes and some find / replace.

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.

@youknowriad
Copy link
Copy Markdown
Contributor

youknowriad commented Mar 24, 2020

Thanks for the PR 👍

Unfortunately, renaming size to padding is a breaking change and ideally we avoid these since the component already ships with WP. Also, I feel it's misleading as well because one could assume that it's CSS padding and provide numbers.

@jasmussen
Copy link
Copy Markdown
Contributor

This seems good to me!

I assume we can't really use the grid-unit variables here? In that case, ship it!

@jameskoster
Copy link
Copy Markdown
Contributor Author

I assume we can't really use the grid-unit variables here?

I don't believe those have been implemented yet?

Also, the padding styles are defined in a .js file, I'm not sure if that would have access to those variables?

@jasmussen
Copy link
Copy Markdown
Contributor

@jameskoster
Copy link
Copy Markdown
Contributor Author

jameskoster commented Mar 25, 2020

Ah, I was looking for the vanilla css vars suggested in the post on make 🤦‍♂

Also, the padding styles are defined in a .js file, I'm not sure if that would have access to those variables?

Do you know if those vars are accessible in card-styles.js? Or how to make them accessible? :D

@jasmussen
Copy link
Copy Markdown
Contributor

No, I don't think they are, and that's fine. But the values you chose are compatible :) that's a great start.

Copy link
Copy Markdown

@ItsJonQ ItsJonQ left a comment

Choose a reason for hiding this comment

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

🚀 from me! This reminds me! In the future, it would be handy to have a js based mixin (maybe something like spacing(3)) to retrieve these values

@ItsJonQ
Copy link
Copy Markdown

ItsJonQ commented Apr 7, 2020

Will merge this when Travis is green and happy
Thanks again for working on this!

@ItsJonQ ItsJonQ merged commit 3ea2489 into WordPress:master Apr 7, 2020
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 7, 2020
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 7, 2020

Congratulations on your first merged pull request, @jameskoster! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 7, 2020
@jameskoster jameskoster deleted the update/Card branch April 8, 2020 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants