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

Comments on README.md #3

Open
feesh opened this issue Oct 15, 2015 · 10 comments
Open

Comments on README.md #3

feesh opened this issue Oct 15, 2015 · 10 comments

Comments

@feesh
Copy link

feesh commented Oct 15, 2015

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
Copy link
Author

feesh commented Oct 15, 2015

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

@nrrrdcore
Copy link
Contributor

Thanks @feesh! This is all such excellent feedback! 🎉

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 💟

@feesh
Copy link
Author

feesh commented Oct 15, 2015

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. 😁 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
Copy link
Author

feesh commented Oct 15, 2015

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
Copy link

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
Copy link
Contributor

jalcine commented Oct 16, 2015

@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 😀.

@nrrrdcore
Copy link
Contributor

@jalcine 🙏

@adekunleoduye
Copy link

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
Copy link
Contributor

@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
Copy link

dmleong commented Oct 17, 2015

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

@jalcine jalcine assigned jalcine and unassigned jalcine Nov 7, 2015
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

No branches or pull requests

6 participants