clef / frontend-styleguide

Styleguide for front-end code @ Clef.
MIT License
58 stars 3 forks source link

Comments on README.md #3

Open feesh opened 8 years ago

feesh commented 8 years ago

Just a few things —

We use hypens to separate two words

Typo on "hyphens"

Utility classes

I'm a little iffy on the utility classes, I personally prefer adding them as mixins so that the HTML doesn't end up littered with a ton of stylistic class additions, but if they're not used hella then maybe they're okay just as classes?

Organize CSS properties into groups according to how they affect the DOM or are loaded on a page. Our compiled CSS actually shows re-ordered CSS properties, so we should always base how we order them in our css files on what's easiest to search for, read, and debug.

These two sentences seem to contradict each other — if the compiler is reordering them anyway, maybe just the second sentence is fine?

Spacing

Are you also specifying two-space indentation? And maybe no white-space at the ends of lines?

Use pixels for font-sizes and whole integers for letter-spacing.

Whole integers for line-height, or do you mean letter-spacing too?

Commenting our CSS

Do you allow Sass comment styles too with //, or do you treat multi-line comments differently?

Images

Maybe a note about using .svg for any vector images or icons?

use the HTML tag or inline images

Nit: double space between "HTML tag"

Also maybe a section about accessibility best practices, like making sure <img> have alt tags, etc? If you're expanding on the Sass syntax prefs, maybe also including recommendations around nesting, folder architecture, etc. :)

http://sass-guidelin.es/ is another good resource!

feesh commented 8 years ago

One more thing: responsive considerations? i.e. should folks write media queries for progressive enhancement, small to big or big to small?

nrrrdcore commented 8 years ago

Thanks @feesh! This is all such excellent feedback! :tada:

Fixed the typos! I was feeling iffy about giving people the go-ahead with utility classes so I removed them and will try to refactor those out of our code at some point. I also went ahead and rewrote the sentence under the class naming section so that it makes sense and doesn't contradict itself (lol at me).

I contributed a sentence about indentation and no trailing white space, too!

I'm not totally sure about commenting, history and visually I feel like the comments with the * are more visually distinctive but would love to chat with more folks on the team about it and see what they're all comfortable with. Thanks for pointing this out!

Maybe a note about using .svg for any vector images or icons?

I need to do this. We have a ton of work to do around standardizing this. Right now I'm thinking about bringing in an icon font to handle icons and svg inline images add requests so I'm not totally sure yet. Will definitely invest more thought around svg going forward.

Going to expand on Accessibility in V2, I've desperately wanted to improve my own skill set in this area so now I have a reason to do some more research and testing.

One more thing: responsive considerations? i.e. should folks write media queries for progressive enhancement, small to big or big to small?

I'm working on building out variable scales that are unique for desktop vs. mobile. This is how Amazon handles it and I'm hoping that we can do the same.

Thank you so much :heart_decoration:

feesh commented 8 years ago

Awesome! This is super fun to read through, thanks for sharing! Working on a styleguide proj at work, too, so pretty cool seeing your process. :grin: Related: https://twitter.com/jina/status/649651827282079744

Re: comments — I got used to the // because they show up when you toggle comments in Sublime Text (⌘ + /), but whatever your team is more comfortable using is prob best. I think we're switching to the triple /// though, because we're considering using Sassdoc... TBD.

I also need to step up my accessibility game, so I'll let you know if I find any great resources. :)

(One more nit, sorry — Sass is capitalized, not uppercased. I think it's not an acronym?) sassattack

feesh commented 8 years ago

Last one I swear: "so we should alway order them" <-- always

I like the way you group your CSS properties, too! Potentially stealing that one...

Fauntleroy commented 8 years ago

I'm not sure if @feesh was getting at this or not, but the documentation says...

Use pixels for font-sizes and whole integers for line-height.

Then clearly contradicts itself in the example:

line-height: 1.5;
jalcine commented 8 years ago

@Fauntleroy commented on Oct 16, 2015, 1:27 PM EDT:

I'm not sure if @feesh was getting at this or not, but the documentation says...

Use pixels for font-sizes and whole integers for line-height.

Then clearly contradicts itself in the example:

line-height: 1.5;

I guess we could update this to say unit-less measurements as opposed to integers :grinning:.

nrrrdcore commented 8 years ago

@jalcine :pray:

adekunleoduye commented 8 years ago

When you say Targeting descendant selectors should be avoided does that include table (when styling tr and td) and ol and ul (when styling li)?

Wouldn't that be redundant in some cases?

nrrrdcore commented 8 years ago

@adekunleoduye I would use your best judgement when it comes to tables and ul/ol. Remember that CSS is parsed from right to left. So, if you're only styling one table or ul/ol it actually sort of makes sense efficiency-wise to style descendants. However, if there are two different styles of tables or list then the browser has to parse things twice. Instead, I usually give a table and/or a list and it's children unique identifiers and apply specific styles to that selector in lieu of using the descendant selectors, which apply to all of these elements in your app.

While I can see the appeal of styling tables with descendants, I almost never style lists with them.

Does that make sense? It's Friday I'm tired as hell, lol.

dmleong commented 8 years ago

Regarding spacing, what guidelines do you have for spacing when nesting?