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.

Sign up for GitHub

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

Jump to bottom

Introduce standard spacing to the theme #425

Merged
merged 2 commits into from
Nov 22, 2018
Merged

Introduce standard spacing to the theme #425

merged 2 commits into from
Nov 22, 2018

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Nov 22, 2018

Resolves #400.

@fbarl fbarl self-assigned this Nov 22, 2018
@fbarl fbarl requested review from bia and guyfedwards November 22, 2018 11:08
@fbarl fbarl force-pushed the 400-standard-spacing branch from e58e0b8 to 26b12ad Compare November 22, 2018 11:09
@bia
Copy link
Contributor

bia commented Nov 22, 2018

I'm surprised by how few changes there are here?
Could you also remove constants.jsx? I think that's in the scope of this pr.

@fbarl
Copy link
Contributor Author

fbarl commented Nov 22, 2018

I'm surprised by how few changes there are here?

This PR just creates the theme, #424 will apply it to all the components in the repo.

Could you also remove constants.jsx? I think that's in the scope of this pr.

That constant is still being used in service-ui repo, but good point - I'll remove it once https://github.com/weaveworks/service-ui/issues/3518 has been addressed 👍

guyfedwards
guyfedwards approved these changes Nov 22, 2018
View reviewed changes
Copy link
Contributor

@guyfedwards guyfedwards left a comment

Choose a reason for hiding this comment

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

Looks great 👍

src/theme/spacings.js Outdated
import { forEach } from 'lodash';

export const spacing = {
none: '0px',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this? and if so would it be better as just 0 seeing as unit is irrelevant and often times you want to explicitly have no border with border: 0 as opposed to a border of 0px width. Semantics ¯\(ツ)/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we really need this?

I put it there just in case, so that if we ever decided to enforce the linting rules, we don't have come back to this just to add the zero :)

...would it be better as just 0 seeing as unit...

Agree, will make the change.

@fbarl fbarl force-pushed the 400-standard-spacing branch from 4fc9665 to 716d1a9 Compare November 22, 2018 13:43
@fbarl fbarl merged commit e54d769 into master Nov 22, 2018
@fbarl fbarl deleted the 400-standard-spacing branch November 22, 2018 13:45
@bia bia mentioned this pull request Nov 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bia bia bia approved these changes

@guyfedwards guyfedwards guyfedwards approved these changes

Assignees

@fbarl fbarl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fbarl @bia @guyfedwards