This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
DESKTOP-129: Responsive Best Practices #165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeffkamo
reviewed
Apr 30, 2018
css/css-best-practices/readme.md
Outdated
| @@ -93,14 +90,18 @@ When authoring CSS, you should be always aware of the selectors that you are cre | |||
|
|
|||
| Strive to create selectors that actually fully describe where it is authored. Put another way, any given selector should tell you which file and where in the file it is written. | |||
|
|
|||
| This can be down by following this simple rule: the first class in a selector is the file it can be found | |||
| This can be down by following this simple rule: the first class in a selector is the file it can be found. | |||
There was a problem hiding this comment.
Looks like an old typo:
This can be
downdone by...
css/introduction/readme.md
Outdated
|
|
||
| These guidelines are a summary of our base principles: Our code bases should all... | ||
|
|
||
| * Be written like a single person typed it | ||
| * Be components first | ||
| * Be mobile-first | ||
| * Be modular–components are better than page specific styles |
There was a problem hiding this comment.
Given the "Be mobile-first" sentence above, it looks like this sentence is meant to read similarly: a continuous thought. But I suspect this is meant to be two thoughts. Can this be clarified? Maybe adding space, or switching to a colon?
| @@ -0,0 +1,153 @@ | |||
| # Responsive Best Practices | |||
|
|
|||
There was a problem hiding this comment.
Other pages have table of contents, can we include one here too?
|
|
||
| Our philosophy is that _the first breakpoint is no breakpoint_. | ||
|
|
||
| There are plenty of good reasons to approach [design](https://www.lukew.com/ff/entry.asp?933) and [content strategy](https://alistapart.com/article/your-content-now-mobile) from this perspective, and the same goes for styling. An experience shouldn't depend on a specific viewport width work well. |
There was a problem hiding this comment.
Missing "to"
An experience shouldn't depend on a specific viewport width to work well.
| 1. All component and sub-component styles, including modifiers, are in one place | ||
| 1. You can easily see what you are building on top of | ||
| 1. Less scrolling and reduced cognitive overhead | ||
| 1. Debugger is simpler |
There was a problem hiding this comment.
DebuggerDebugging is simpler
|
|
||
| Instead of: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Might be nice to add scss to these block for syntax highlighting.
|
|
||
| Instead of: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Can add scss for nicer syntax highlighting.
css/sass-best-practices/readme.md
Outdated
|
|
||
|
|
||
| ## Global vs. Local Variables/Mixins | ||
|
|
||
| Any `$variable` that is used in more than one file should be placed in the `/vellum/variables.scss` file. Others should be placed at the top of the file in which they're used. | ||
| Any `$variable` that is used in more than one file should be placed in the `/variables.scss` file. Others should be placed at the top of the file in which they're used. |
There was a problem hiding this comment.
_variables.scss (prefixed with underscore). Since it's not a directory, it might not need the slash.
jbandelin
reviewed
Apr 30, 2018
| @@ -308,9 +308,9 @@ Sometimes there are situations when a component makes use of other components, a | |||
| </button> | |||
| ``` | |||
|
|
|||
| In situations like this it is tempting to just style the icon's class inside of the button. However, this practice is poor and creates tight coupling between the Button and Icon components that shouldn't exist. As a rule of thumb, a component should only know about what it's responsible for; it should be unaware of anything external to itself. Continuing with this example, the Icon is an external component, therefore the Button component should be completely unaware there there is even such thing as icon classes, like `c-icon`. | |||
| In situations like this it is tempting to just style the icon's class inside of the button. However, this practice is poor and creates tight coupling between the Button and Icon components that shouldn't exist. As a rule of thumb, a component should only know about what it's responsible for; it shouldn't be aware of anything external to itself. Since Icon is an external component, the Button component should be completely unaware that of `c-icon`. | |||
There was a problem hiding this comment.
Should this say: "...should be completely unaware that of c-icon."
jeffkamo
approved these changes
May 1, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that we're building more mobile/tablet/desktop sites, it's time to put our responsive best practices into our code style docs. These will be updated a bit more after we determine our final grid implementation strategy.
I've also updated our docs a bit in reference to React, and de-emphasized the Adaptive.js specific examples.
Changes
How To Test
TODOs: