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

Include prettier fonts #248

Closed
wants to merge 6 commits into from

Conversation

jacobbednarz
Copy link
Contributor

  • Adds consistent font
  • CSS tidy up

This standardises the font stack to be the same across the board.
This is just a general tidy up to fix up the loose indents, spacing and conventions.
@GrahamCampbell
Copy link
Contributor

Why have you changed the indentation? Can you clean it up, while retaining 4 spaces for indentation?

@jacobbednarz
Copy link
Contributor Author

The file itself didn't show a particular way so I just went with my usual defaults. Happy to update this to be 4 space indents.

@GrahamCampbell
Copy link
Contributor

Looking at the original file in blame mode, it's clear that it should have been 4 spaces. Other contributors seem to have used different indentations.

@denis-sokolov
Copy link
Collaborator

It's really doesn't matter what's the existing style, even if it's messy as it's in this file.
It's preferred if your PR only contains commits directly related to the PR, in this case, it's 98cae75.

All the rest is suggesting cleanup. And while it's possibly a very good suggestion, it's not helping to have to decide on it together with the great suggestion of adding fonts.

@jacobbednarz
Copy link
Contributor Author

Looking at the original file in blame mode, it's clear that it should have been 4 spaces. Other contributors seem to have used different indentations.

While this is true I don't think it should be the contributors responsibility to sift though blame views and history in order to find out which coding style was used first. If it's not evident from your code or guidelines which is the preferred style then I think you can't expect people to pick up in this.

All the rest is suggesting cleanup. And while it's possibly a very good suggestion, it's not helping to have to decide on it together with the great suggestion of adding fonts.

Understood. This type of thing I would usually break up but as the changes were more formatting related, I just separated them into individual commits. If you really want these broken up into their own pull requests I can also do that. Will keep this in mind for future pull requests.

@jacobbednarz
Copy link
Contributor Author

Updated to use 4 space indents.

@denis-sokolov
Copy link
Collaborator

I have merged in commits 98cae75 and 71103c1, the actual fonts stuff, in 542400d.
Thank you for your contribution, this was a good idea!

Sorry, but I won't be merging cleanup commits in.
That's a lot of reformatting (and thus, complicating merges and blames) for no obvious benefit.

@jacobbednarz jacobbednarz deleted the include-prettier-fonts branch November 30, 2014 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants