Add constant spacing #432

SaraVieira · 2017-05-06T17:36:52Z

This PR adds constant spacing throughout the project with an 8px grid.

I did this by adding the spacing variables to the theme like so:

const spacingFactor = 8;
export const spacing = {
	space4: `${spacingFactor / 2}px`,  // 4
	space8: `${spacingFactor}px`,      // 8
	space16: `${spacingFactor * 2}px`,  // 16
	space24: `${spacingFactor * 3}px`,  // 24
	space32: `${spacingFactor * 4}px`,  // 32
	space40: `${spacingFactor * 5}px`,  // 40
	space48: `${spacingFactor * 6}px`,  // 48
};

These were the only spaces I used in the entire project to main consistency.

codecov-io · 2017-05-06T18:09:35Z

Codecov Report

Merging #432 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #432      +/-   ##
=========================================
- Coverage   94.24%   94.2%   -0.05%     
=========================================
  Files          78      78              
  Lines        1113    1121       +8     
  Branches      246     247       +1     
=========================================
+ Hits         1049    1056       +7     
- Misses         60      61       +1     
  Partials        4       4

Impacted Files	Coverage Δ
src/rsg-components/Section/SectionRenderer.js	`100% <100%> (ø)`	⬆️
src/rsg-components/Logo/LogoRenderer.js	`100% <100%> (ø)`	⬆️
...omponents/ReactComponent/ReactComponentRenderer.js	`100% <100%> (ø)`	⬆️
...ponents/PlaygroundError/PlaygroundErrorRenderer.js	`100% <100%> (ø)`	⬆️
src/rsg-components/Markdown/Markdown.js	`90.47% <100%> (ø)`	⬆️
...ponents/TableOfContents/TableOfContentsRenderer.js	`100% <100%> (ø)`	⬆️
...rc/rsg-components/Playground/PlaygroundRenderer.js	`90.9% <100%> (ø)`	⬆️
src/rsg-components/Methods/MethodsRenderer.js	`100% <100%> (ø)`	⬆️
src/styles/theme.js	`100% <100%> (ø)`	⬆️
src/rsg-components/Message/MessageRenderer.js	`100% <100%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccfb8e8...a9a8c24. Read the comment docs.

sapegin · 2017-05-07T09:22:14Z

src/rsg-components/Heading/HeadingRenderer.js

-		marginLeft: -30,
-		paddingLeft: 30,
+		marginLeft: -32,
+		paddingLeft: spacing.space32,


I think they both should you the same variable.

sapegin · 2017-05-07T09:25:56Z

src/rsg-components/Methods/Methods.css

@@ -9,19 +9,19 @@
 .tableBody {


We don’t use this file, could you please remove it? Merge artifact ;-/

sapegin · 2017-05-07T09:26:55Z

src/rsg-components/Methods/MethodsRenderer.js

-		paddingRight: 15,
-		paddingBottom: 6,
+		paddingRight: spacing.space16,
+		paddingTop: spacing.space8,


Looks like a typo, it should be bottom padding.

Sorry , typo

sapegin · 2017-05-07T09:27:26Z

src/rsg-components/PlaygroundError/PlaygroundErrorRenderer.js

 	root: {
-		margin: [[-15, -15, -18]],
+		margin: [[-16, -16, -16]],


space16? But actually this numbers are not random — the red background should fill the playground container.

I will change them back and add that as a comment

sapegin · 2017-05-07T09:29:11Z

src/styles/theme.js

@@ -10,7 +12,17 @@ export const type = '#b77daa';
 export const baseBackground = '#fff';
 export const errorBackground = '#c00';
 export const codeBackground = '#f5f5f5';
-export const font = ['-apple-system', 'BlinkMacSystemFont', '"Segoe UI"', '"Roboto"', '"Oxygen"', '"Ubuntu"',
+export const font = ['"Roboto"', '-apple-system', 'BlinkMacSystemFont', '"Segoe UI"', '"Roboto"', '"Oxygen"', '"Ubuntu"',


Why Roboto?

Because it's pretty. That's all :p

We’re using system fonts.

They are available on all platforms, they look good (especially SF ;-) and they are used in the OS UI.

How many people have Roboto installed? :-)

Not many , but for those who don't we can just go to the apple-system ones

Sorry, I don’t see any reason to use Roboto just for a few people who have it, while system fonts are still better.

sapegin · 2017-05-07T09:30:21Z

src/styles/theme.js

 	'"Cantarell"', '"Fira Sans"', '"Droid Sans"', '"Helvetica Neue"', 'sans-serif'];
 export const monospace = ['Consolas', '"Liberation Mono"', 'Menlo', 'monospace'];
 export const small = '@media (max-width: 600px)';
+export const sidebarWidth = 200;
+export const spacing = {


I’d use space.0, space.1, space.2. Actual numbers are implementation detail, so it’s a bit like red = blue thing ;-)

I did it like this to be easier to make sense of it, so when someone sees space16 they will see its 16 px

In that case you need to remember that and calculate the right number every time, with 1, 2, 3 it easy: single space, double space…

But the first one is half , how should I call it ?

Spacing renamed to:

export const spacing = { 0: `${spacingFactor / 2}px`, // 4 1: `${spacingFactor}px`, // 8 2: `${spacingFactor * 2}px`, // 16 3: `${spacingFactor * 3}px`, // 24 4: `${spacingFactor * 4}px`, // 32 5: `${spacingFactor * 5}px`, // 40 6: `${spacingFactor * 6}px`, // 48 };

I also exported the spacingFactor if someone wants to change it in their project

I’d call it 0: https://medium.freecodecamp.com/css-in-javascript-the-future-of-component-based-styling-70b161a79a32

sapegin · 2017-05-07T09:31:03Z

src/rsg-components/ComponentsList/ComponentsListRenderer.js

 		fontFamily: font,
-		fontSize: 15,
+		fontSize: spacing.space16,


We shouldn’t use spacers for font sizes. But would be nice to have variables for that too.

Typo again, but I can add some :)

SaraVieira · 2017-05-07T11:27:40Z

@sapegin Most of the requested changes were made and I added font variables too :)

SaraVieira · 2017-05-07T18:39:40Z

@sapegin All changed made. may I merge ?

sapegin · 2017-05-07T21:48:15Z

Merged, thanks!

1. More consistent naming. 2. Use numbers instead of strings for spacing. 3. Group font families, media queries, colors. 4. Variable for border radius. 5. Fix bugs introduced in the previous commit.

sapegin · 2017-05-08T09:38:21Z

I’ve made some tweaks in 19ed879 ;-)

@SaraVieira

1. Variables for sidebar background and max width. 2. Fix error color. @SaraVieira 👻

Add constant spacing

4134543

SaraVieira requested review from okonet and sapegin May 6, 2017 17:36

sapegin requested changes May 7, 2017

View reviewed changes

requested changes

90af30a

Sara Vieira added 3 commits May 7, 2017 12:39

rename spacing

7ab7577

rename spacing

46e8cf5

fix fonts

a9a8c24

sapegin merged commit f4e894a into master May 7, 2017

sapegin deleted the spacing branch May 7, 2017 21:47

sapegin pushed a commit that referenced this pull request May 8, 2017

Refactor: Extract spacing and fonts to theme (#432)

41bc0ff

sapegin added a commit that referenced this pull request May 8, 2017

Refactor: More theme tweaks (#432)

412a708

1. Variables for sidebar background and max width. 2. Fix error color. @SaraVieira 👻

This was referenced Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#165

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#166

Open

kopax mentioned this pull request Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities rollup-umd/react-styleguidist#132

Open

This was referenced Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#175

Open

[Snyk] Fix for 1 vulnerabilities Bhanditz/react-styleguidist#176

Open

kopax mentioned this pull request Dec 26, 2022

[Snyk] Fix for 1 vulnerabilities rollup-umd/react-styleguidist#142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add constant spacing #432

Add constant spacing #432

SaraVieira commented May 6, 2017

codecov-io commented May 6, 2017 •

edited

Loading

sapegin May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

SaraVieira May 7, 2017 •

edited

Loading

SaraVieira May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

sapegin May 7, 2017

sapegin May 7, 2017

SaraVieira May 7, 2017

SaraVieira commented May 7, 2017

SaraVieira commented May 7, 2017 •

edited

Loading

sapegin commented May 7, 2017

sapegin commented May 8, 2017

Add constant spacing #432

Add constant spacing #432

Conversation

SaraVieira commented May 6, 2017

codecov-io commented May 6, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaraVieira May 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SaraVieira commented May 7, 2017

SaraVieira commented May 7, 2017 • edited Loading

sapegin commented May 7, 2017

sapegin commented May 8, 2017

codecov-io commented May 6, 2017 •

edited

Loading

SaraVieira May 7, 2017 •

edited

Loading

SaraVieira commented May 7, 2017 •

edited

Loading