-
Notifications
You must be signed in to change notification settings - Fork 188
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
Clean up rheostat css theming (3/7) #163
Conversation
c52beca
to
e63b09e
Compare
src/DefaultHandle.jsx
Outdated
@@ -52,10 +48,13 @@ DefaultHandle.propTypes = propTypes; | |||
|
|||
DefaultHandle.defaultProps = defaultProps; | |||
|
|||
const DEFAULT_HANDLE_WIDTH = 1.9 * 0.8; |
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.
with theming, would we be able to define these values in the theme, so users could define their own default handle width?
Separately, where does 1.9 and 0.8 come from?
5ecf6e1
to
3d1ac3c
Compare
e63b09e
to
de87561
Compare
3d1ac3c
to
3f3e713
Compare
de87561
to
daedb98
Compare
3f3e713
to
8b73a3f
Compare
daedb98
to
cf46dd1
Compare
ca50ace
to
7f4a8eb
Compare
8cd1c2f
to
cdf7134
Compare
src/DefaultHandle.jsx
Outdated
@@ -65,7 +61,7 @@ export default withStyles(({ color, unit }) => ({ | |||
zIndex: 2, | |||
boxShadow: `0 ${unit / 4}px ${unit / 4}px ${color.textDisabled}`, | |||
':focus': { | |||
boxShadow: `${color.focus} 0 0 2px 2px`, | |||
boxShadow: `${color.teal} 0 0 1px 1px`, |
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 think we might want to keep this as color.focus
and just have it map to teal in the theme so that the variable is meaningful as to what it represents.
src/themes/DefaultTheme.js
Outdated
color: { | ||
black: 'black', | ||
white: '#fcfcfc', | ||
grey: '#d8d8d8', | ||
teal: '#abc4e8', | ||
textDisabled: '#DBDBDB', |
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.
nit: lowercase
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.
LGTM after we squash down these two commits (they don't seem very meaningful separately)
7f4a8eb
to
e5cb7f8
Compare
e70531a
to
9741852
Compare
9741852
to
926dc37
Compare
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.
We do need to remove clean-css
from the deps, but my other comments are probably not entirely necessary. Just worth thinking about.
package.json
Outdated
@@ -71,6 +71,7 @@ | |||
}, | |||
"dependencies": { | |||
"airbnb-prop-types": "^2.10.0", | |||
"clean-css": "^4.1.11", |
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 looks like a mistake
src/DefaultProgressBar.jsx
Outdated
@@ -60,7 +60,7 @@ export default withStyles(({ color, unit }) => ({ | |||
}, | |||
|
|||
DefaultProgressBar_progressBar: { | |||
backgroundColor: color.teal, | |||
backgroundColor: color.focus, |
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 think here this should maybe be color.progressBar
and point back to the teal? I don't know that there's an inherent relationship between focus state and progress bar color and we should the ability to override individually in the theme.
src/themes/DefaultTheme.js
Outdated
color: { | ||
black: 'black', | ||
white: '#fcfcfc', | ||
grey: '#d8d8d8', | ||
teal: '#abc4e8', | ||
focus: '#abc4e8', |
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 think what I meant instead is that we would have:
teal: '#abc4e8',
and then also have:
focus: this.teal,
progressBar: this.teal,
or whatever.
Alternatively, you could have:
const core = {
black: 'black',
white: '#fcfcfc',
grey: '#d8d8d8',
teal: '#abc4e8',
lightgrey: '#dbdbdb',
};
const DefaultTheme = {
...
color: {
...core,
focus: core.teal,
textDisabled: core.lightgray,
...
},
};
but maybe that's overengineering it and not super helpful.
926dc37
to
d39970f
Compare
d39970f
to
8eaa55e
Compare
This commit sits on top of #162, and will be rebased when that commit is merged.
Cleans up CSS/ the default theme for Rheostat.