-
Notifications
You must be signed in to change notification settings - Fork 27
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
Weight loss program #787
Weight loss program #787
Conversation
490c55b
to
2837961
Compare
@tristen @davidtheclark this is ready for review. |
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.
Changes look great!
Decided against drastic conceptual changes
By that do you mean reducing color variations on form elements to two states?
site/catalog/toggles.js
Outdated
@@ -84,12 +78,12 @@ class Toggles extends React.Component { | |||
</div> | |||
|
|||
<div className='mb12 txt-bold color-darken50 txt-uppercase txt-s'>Light variations</div> | |||
<div className='bg-gray round p12y mt12'> | |||
<div className='bg-gray round px12 py12y mt12'> |
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.
Should be py12
site/catalog/toggles.js
Outdated
{lightenColors.map((c) => <ToggleEl key={c} color={c} />)} | ||
</div> | ||
|
||
<div className='mt24 mb12 txt-bold color-darken50 txt-uppercase txt-s'>Disabled and light</div> | ||
<div className='bg-gray round p12y mt12'> | ||
<div className='bg-gray round px12 py12y mt12'> |
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.
Should be py12
site/examples/progress_bars.js
Outdated
@@ -9,7 +9,7 @@ const playback = `<div class='flex-parent flex-parent--row flex-parent--center-c | |||
<button class='flex-child py18 pr12 pl18 border-r border--gray-light bg-white bg-green-faint-on-hover cursor-pointer round-l'> | |||
<div class='triangle--r triangle color-green color-green'></div> | |||
</button> | |||
<div class='flex-child flex-child--grow p18 bg-white'> | |||
<div class='flex-child flex-child--grow px18 py18bg-white'> |
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.
Missing space between class names
site/examples/tooltips.js
Outdated
<span class='flex-child triangle triangle--d'></span> | ||
</div>`; | ||
|
||
const basic_right = `<div class='flex-parent-inline flex-parent--center-cross'> | ||
<span class='flex-child triangle triangle--l'></span> | ||
<div class='flex-child p6 round txt-bold txt-s align-center bg-darken75 color-white'>Hello world!</div> | ||
<div class='flex-child px6 py6round txt-bold txt-s align-center bg-darken75 color-white'>Hello world!</div> |
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.
Missing space between class names
site/examples/tooltips.js
Outdated
</div>`; | ||
|
||
const basic_left = `<div class='flex-parent-inline flex-parent--center-cross'> | ||
<div class='flex-child p6 round txt-bold txt-s align-center bg-darken75 color-white'>Hello world!</div> | ||
<div class='flex-child px6 py6round txt-bold txt-s align-center bg-darken75 color-white'>Hello world!</div> |
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.
Missing space between class names
site/examples/tooltips.js
Outdated
<span class='flex-child triangle triangle--r'></span> | ||
</div>`; | ||
|
||
const basic_with_closure = `<div class='flex-parent-inline flex-parent--center-cross flex-parent--column'> | ||
<div class='flex-child relative p18 round txt-bold txt-s align-center bg-darken75 color-white'> | ||
<div class='flex-child relative px18 py18round txt-bold txt-s align-center bg-darken75 color-white'> |
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.
Missing space between class names
border: 1px solid var(--lighten10); | ||
line-height: 18px; | ||
border-radius: 3px; | ||
padding: 2px 3px; |
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.
Was dropping these properties intentional?
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.
Yeah, prose--dark is a modifier for prose so no need to duplicate these rules.
src/variables.json
Outdated
"neutral45": "rgba(127, 127, 127, 0.45)", | ||
"disabled-primary-interactive-color": "rgba(127, 127, 127, 0.25)", | ||
"disabled-secondary-interactive-color": "rgba(127, 127, 127, 0.1)", | ||
"disabled-text-color": "rgba(127, 127, 127, 0.45)", |
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.
Is disabled
accurate? This is used for placeholder too right? And I'm not sure how primary
& secondary
align with how they are used. How about input-{value}-on-blur
?
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.
Ohh that is the one inaccurate use. I can change this to be 'placeholder-text-color'
'gray', | ||
'gray-dark', |
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.
Curious about all these gray-{varient}
omissions? They ... still exist but you're removing them from the catalog?
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.
This was a fix - we never had -dark variants here.
c3c3e28
to
bdec5fa
Compare
Addressed feedback! |
'darken10', | ||
'darken25', | ||
'darken50', | ||
'darken75', | ||
|
||
'lighten5', |
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 like these cuts!
scripts/build-color-variants.js
Outdated
|
||
function isNotSuitableForButtons(color) { | ||
// Filled Buttons should be allowed to have subtle backgrounds within reason. | ||
return color === 'black' || /(-faint|-dark)$/.test(color); | ||
} |
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.
🎉
|
||
input:checked + .switch--dot-${color}::after { | ||
background-color: var(--${color}); | ||
} | ||
`); | ||
}, ''); | ||
}; | ||
|
||
variantGenerators.range = function (colors) { |
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.
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.
Good catch - was from an earlier refactor attempt. I removed this class but didn't remove the documentation?
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.
Done!
scripts/build-color-variants.js
Outdated
// Do not create form elements from values that are too light or too dark, | ||
// for accessibility and to save space. | ||
return color === 'black' || /^(darken10|lighten10)$/.test(color) || /(-dark|-light|-faint)$/.test(color); | ||
} |
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.
What do you about think -faint
for hover and active states? Should we keep those, or also remove them to encourage stronger contrast?
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'd be in favor of cutting them - can add that to this PR
Since a lot changes here, might be useful to add a Changelog modifications in this PR. |
- rename disabled-text-color to more appropriate inactice-text-color, used for both disabled and placeholder text - fix find and replace typos
360e1a7
to
a9a7242
Compare
@tristen @davidtheclark oookay i think this is ready for a final 👀 then merge |
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.
All these changes seem wonderful to me! Very fine work.
See changes in changelog.md
* adjust icon sizing, add icon-wrapper class, closes #763 * rename icon-wrapper to icon-inliner, other small fixes * force txt-bold to be 1em line height, fixes line height offsets, closes #775 * rework size scale, closes #749 * Add limiter class, consistify scale even more * only single-declaration utility classes should have important (#785) * New copy of Open Sans (#786) Reverts prior line-height change to bold weight. Closes #775. * reset abbr text-decoration, fixes txt-abbr rule (#788) * Weight loss program (#787) See changes in changelog.md * Lighten gray-faint and add a line of caution to color documentation * Reintroduce darken5 and lighten5 for backgrounds only * Add icons option to build script (#794) options.icons is an array of icon names. If this option is used, only icon names that match options.icons will be included in Assembly. Closes #782 * adds color and hover state to prose links (#798) * Make uglify-js a real dependency Closes #800. * add unprose class, closes #793 (#795) * add select--xs class to match btn--xs (#811) * fix spin animation utility (#812) closes #790 * prepare v0.14.0 (#813)
This is ready to go. Decided against drastic conceptual changes.
// mb-pages
CSS: 262 kB (minified) => 29.7 kB (gzipped)
SVG: 50 kB => 15.1 kB (gzipped)
// This branch
CSS: 172 kB (minified) => 21.9 kB (gzipped)
SVG: 50 kB => 15.1 kB (gzipped)
This branch turned into a larger v1 push, beyond just space saving. Changes:
btn--stroked--2
andselect--stroked--2
modifier classes to create stroked buttons and select elements that have 2px strokes.neutral
variables to more meaningfuldisabled-{type}-interactive
variables.gray-faint
color to #e5e5e5 so it's legible on a wider range of monitors.darken5
andlighten5
color variations to save space and encourage more accessible design.p{n}
andm{n}
rules.-faint
or-light
form elements are no longer available for any element, anddarken10
anddarken10
are no longer available for any for element except buttons.