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

add house style document, put a link to it in other docs #158

Merged
merged 5 commits into from
Mar 24, 2017

Conversation

hollsk
Copy link
Contributor

@hollsk hollsk commented Mar 23, 2017

First shot at creating a house style doc for #151

(also includes a URL fix in css.md)

Copy link
Contributor

@andrewmee andrewmee left a comment

Choose a reason for hiding this comment

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

It's so good that I feel it could be a bit wasted languishing in the practices directory... I wonder if we couldn't put this front and centre in the readme (and just incorporate those few things from the current readme that his doesn't currently cover).

I wonder if others agree?

@jpw
Copy link
Contributor

jpw commented Mar 24, 2017

Minor niggles aside, vg ty :)


### Linting

We lint our CSS, JavaScript, and templates written with DustJS. This helps us to keep our codebases consistent, avoid common logic and implementaton errors, and lets us concentrate on solving problems instead of formatting.
Copy link
Contributor

@jpw jpw Mar 24, 2017

Choose a reason for hiding this comment

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

typo "implementaton"

@@ -727,6 +728,7 @@ But in the following example we would not use it:
[Nature playbook(private)]: https://github.com/nature/playbook
[bem]: https://github.com/springernature/frontend/practices/bem-css.md
[a hybrid of OOCSS and Atomic CSS]: https://github.com/springernature/frontend/practices/mosaic-css.md
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should remove ref to Atomic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify what you mean? 😁 is there a page about atomic, or something else?

(that "hybrid of" link is really confusing as it exists as a reference but isn't actually used by anything, and i don't know enough about the CSS stack here to be able to accurately fix things)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant change it to something mentioning "utility classes" not Atomic, as we agreed that this new fangled thing called Atomic CSS at acss.io was a possible sauce of confusion ;)

Also I think the link should be;
https://github.com/springernature/frontend-playbook/blob/master/practices/mosaic-css.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bleurgh. You know what, I'm just gonna delete it entirely. It's unused in the text, so it can go. If somebody needs to refer to it they can add some actual text and then put the link ref back in 😁

@@ -38,7 +37,7 @@ General Principles

### Code For Humans

Your JavaScript should always be optimised for readability first, someone is going to have to maintain your code. Most of the rules in this style guide are here to help enforce this.
Your JavaScript should always be optimised for readability first, someone is going to have to maintain your code. Most of the rules in this style guide are here to help enforce this. See our [house style document](../practices/house-style.md) to understand the rationale behind enforcement of style.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest "first, as someone" or "first—someone"

@jpw
Copy link
Contributor

jpw commented Mar 24, 2017

@andrewmee good point, I do think this deserves more prominence somehow -- it is a nice intro to how & why we do things. I do feel hitting the playbook then browsing the (unindexed) dirs is a bit "unguided".

@hollsk
Copy link
Contributor Author

hollsk commented Mar 24, 2017

I agree that the unindexed dirs is a problem that's getting bigger as more information is added. I don't think that the house style guide should be in the readme, as it's a tiny bit too specific.

I thought last night that it'd be nice to have a "Where do I start" section in the readme, that would provide a path through the content. I haven't yet decided if that should be a full table of contents or just some pointers, maybe with a separate ToC on another page. Leaning towards the separate ToC + starting pointers, so we can keep the readme lean. Thoughts?

@jpw
Copy link
Contributor

jpw commented Mar 24, 2017

Hmmms, how about a "Where do I start" header in the readme, and underneath "Have look at our House Style Guide" (but probably with better wording) might be an idea..? I think the House Style Guide would make a great intro!

And maybe having an index page for each directory with a quick summary of the linked-to docs, but that would be another ticket i guess.

@jpw
Copy link
Contributor

jpw commented Mar 24, 2017

👍

@jpw
Copy link
Contributor

jpw commented Mar 24, 2017

even more 👍

@hollsk
Copy link
Contributor Author

hollsk commented Mar 24, 2017

Right, I think I'm done here. Will worry about indexing in another ticket like Jon suggests.

Review pls kthx

@jpw
Copy link
Contributor

jpw commented Mar 24, 2017 via email

@hollsk hollsk merged commit 3b65937 into master Mar 24, 2017
@hollsk hollsk deleted the generalisetech branch March 24, 2017 14:01
@dotcode
Copy link
Contributor

dotcode commented Apr 5, 2017

How did I miss this?!? This is fantastic! ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants