Skip to content
This repository was archived by the owner on Nov 5, 2024. It is now read-only.

Added html and css to style guide - #17

Closed
ghost wants to merge 2 commits into
masterfrom
html-css
Closed

Added html and css to style guide#17
ghost wants to merge 2 commits into
masterfrom
html-css

Conversation

@ghost

@ghost ghost commented Oct 26, 2016

Copy link
Copy Markdown

@cfnelson cfnelson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity have you looked at or used css-modules or radium though radium is react specific. Kind of like css-modules as it isn't framework dependent.

Comment thread style-guide/code-style/sass/README.md Outdated
* Use hex values for colors when possible.
* Don't specify a unit after using a 0 value.
* Avoid nesting more than 2 levels, unless pseudo classes.
* Use

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Use" ???

Comment thread style-guide/code-style/html/README.md Outdated
# HTML

## Formatting
* Don't use inline styles, unless you are generating dynamic styles in your templating system.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some Inlining of styles is recommended or advocated for performance. e.g - displaying above the fold content. Check here and let me know what you think ? Can maybe add some reference or content from this ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should always start with the most maintainable thing until/unless there's a specific need for some sort of gnarly optimization. I think @oliverthomasklein is trying to write general guidelines.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agreed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍

@cfnelson

cfnelson commented Oct 27, 2016

Copy link
Copy Markdown
Contributor

Also, I do not know enough on HTML/CSS to give a strong/assertive opinion on best style/practices. Should get @karldanninger in on this review :) Nice work otherwise :)

@karldanninger

Copy link
Copy Markdown

Hehe, I might start advocating we write all of our styles in JavaScript

Comment thread style-guide/code-style/html/README.md Outdated
# HTML

## Formatting
* Don't use inline styles, unless you are generating dynamic styles in your templating system.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should always start with the most maintainable thing until/unless there's a specific need for some sort of gnarly optimization. I think @oliverthomasklein is trying to write general guidelines.

Comment thread style-guide/code-style/html/README.md Outdated

## Formatting
* Don't use inline styles, unless you are generating dynamic styles in your templating system.
* Only use ID's to be targeted my JS.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

by JS

@pauldowman

Copy link
Copy Markdown
Contributor

Instead of a sass style guide should it just be for CSS in general?

@ghost

ghost commented Oct 27, 2016

Copy link
Copy Markdown
Author

@pauldowman I was pretty unsure on this. A lot of the formatting is redundant if you are using CSS. I looked at how Thoughtbot did it, they used sass. I think it's more accurate.

@cfnelson

Copy link
Copy Markdown
Contributor

@oliverthomasklein @karldanninger This has been sitting around for quite some time. So long that we are now using Styled Components. 😂

Would love to get this refactored and merged in. Happy to help and provide my perspective as well to help push this along.

Some suggestions:

  • Don't delete the Readme.md, the files seem short enough to exist in the one file under their own headings! 💯 If it get's large we can add a table of contents. 🍡
  • We should add a section for CSS in JSS 🎋 e.g - styled-components, emotion, etc.
  • SASS should become CSS Preprocessors (less/sass/stylus) ..etc? 🥇
  • Or SASS could become a sub-heading to CSS Prepocessors if you wanted specific recommendations instead of generalised ones?

Going to pull in @xavcz as well as I am sure he will have/add some 🔥 and insight. Would you be interested in giving any thoughts or dot points xavs? Will be happy to type/clean them up for you as I know you are extremely busy. 👷

@xavxyz

xavxyz commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

🤐 no opinion. Use whatever fits the job, the project structure & the team: don't be dogmatic, there is no 1 solution fit all.

@cfnelson

cfnelson commented Oct 12, 2017

Copy link
Copy Markdown
Contributor

@xavcz I might have not communicated the purpose of this PR properly. Sorry 😞 . This section of the OK GROW! guides is our Style Guide. It states our opinion on best practices for how to structure/format/design your app when writing html/css/(css preprocessor/css in jss) etc.

Even your comment of don't be dogmatic, there is no 1 solution fit all is a good point to make and should be mentioned somewhere. 🎉 😄

@xavxyz

xavxyz commented Jan 12, 2018

Copy link
Copy Markdown
Contributor

Closing, but content should be re-used!

@xavxyz xavxyz closed this Jan 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants