-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Docs: add "components.md" documentation. #427
Conversation
|
||
We'd call one "component" and the other "sub-component". | ||
|
||
https://cloudup.com/c5hdOQ5jlIB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does GitHub render this properly? Shouldn’t it be: ![hi!](https://cldup.com/CP_Z6Cqec--3000x3000.png)
?
This document is meant to communicate how we structure the UI in components, and how composing relates to the CSS and className guidelines.
928bafe
to
ec8432b
Compare
|
||
Imagine you want to change the border color of a `SiteIcon` component when it is displayed in the sidebar of `my-sites`. That `SiteIcon` component happens to be rendered within a `Site`, within a `SiteSelector`, and finally in a `Sidebar`. | ||
|
||
If we were doing inline styles it would mean we need to pass down a style prop from `sidebar` (the component that wants to make the modification) all the way down to `site-icon` to modify that specific border style value. Which is messy, obscure, and requires passing down a meaningless attribute through components that don't care about it, coupling them with a design intent you want to express in the "Sidebar". Now, with CSS and our naming guidelines it becomes just an old `.sidebar .site-icon {}` on the sidebar's `style.scss` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with one aspect of this. I think it would be better for the .sidebar .site-icon {}
directive to live in the site-icon's style.scss
file. So all of the styles related to site-icon
are in one place and very easy to see and be aware of together. You also can see that some change you are trying to make would potentially be out-specified in certain cases. Whereas you would likely miss that otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning is that .sidebar .site-icon {}
is really an aspect of the sidebar, not of site-icon
, and should be placed together with the design aspects that make the sidebar. There's also no reason for site-icon
to have any awareness of parent intentions. Another aspect of this is that if we start compiling separate .css files based on components used, any component using site-icon
would be compiling styles for sidebar
, which they shouldn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider all the positioning/color changes done to .gridicon
. It would be incredibly messy to have all of that in gridicons/style.scss
alright, let's get this merged. |
This document is meant to communicate how we structure the UI in components, and how composing relates to the CSS and className guidelines.