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

Docs: add "components.md" documentation. #427

Merged
merged 2 commits into from
Nov 23, 2015
Merged

Docs: add "components.md" documentation. #427

merged 2 commits into from
Nov 23, 2015

Conversation

mtias
Copy link
Member

@mtias mtias commented Nov 22, 2015

This document is meant to communicate how we structure the UI in components, and how composing relates to the CSS and className guidelines.

@mtias mtias added Documentation Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 22, 2015

We'd call one "component" and the other "sub-component".

https://cloudup.com/c5hdOQ5jlIB
Copy link
Member

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)?

@nb nb added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 22, 2015
This document is meant to communicate how we structure the UI
in components, and how composing relates to the CSS and className guidelines.

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

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.

Copy link
Member Author

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.

Copy link
Member Author

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

@rralian
Copy link
Contributor

rralian commented Nov 23, 2015

alright, let's get this merged.

nb added a commit that referenced this pull request Nov 23, 2015
Docs: add components documentation
@nb nb merged commit 9d39e42 into master Nov 23, 2015
@mtias mtias deleted the add/components-doc branch November 23, 2015 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants