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

Clean up rheostat css theming (3/7) #163

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

philipfweiss
Copy link
Contributor

This commit sits on top of #162, and will be rebased when that commit is merged.

Cleans up CSS/ the default theme for Rheostat.

@philipfweiss philipfweiss requested review from majapw and ljharb June 29, 2018 18:19
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from c52beca to e63b09e Compare June 29, 2018 18:32
@philipfweiss philipfweiss changed the title Clean up rheostat css theming Clean up rheostat css theming (3/7) Jun 29, 2018
@philipfweiss philipfweiss changed the base branch from master to pw--rheostat-add-css-building-scripts June 29, 2018 23:30
@@ -52,10 +48,13 @@ DefaultHandle.propTypes = propTypes;

DefaultHandle.defaultProps = defaultProps;

const DEFAULT_HANDLE_WIDTH = 1.9 * 0.8;
Copy link
Collaborator

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?

@philipfweiss philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch from 5ecf6e1 to 3d1ac3c Compare July 1, 2018 21:07
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from e63b09e to de87561 Compare July 1, 2018 21:13
@philipfweiss philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch from 3d1ac3c to 3f3e713 Compare July 1, 2018 21:30
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from de87561 to daedb98 Compare July 1, 2018 21:33
@philipfweiss philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch from 3f3e713 to 8b73a3f Compare July 1, 2018 21:49
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from daedb98 to cf46dd1 Compare July 1, 2018 21:51
@philipfweiss philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch 2 times, most recently from ca50ace to 7f4a8eb Compare July 1, 2018 21:56
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch 2 times, most recently from 8cd1c2f to cdf7134 Compare July 1, 2018 22:05
@@ -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`,
Copy link
Collaborator

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.

color: {
black: 'black',
white: '#fcfcfc',
grey: '#d8d8d8',
teal: '#abc4e8',
textDisabled: '#DBDBDB',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lowercase

Copy link
Collaborator

@majapw majapw left a 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)

@philipfweiss philipfweiss force-pushed the pw--rheostat-add-css-building-scripts branch from 7f4a8eb to e5cb7f8 Compare July 2, 2018 18:49
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch 3 times, most recently from e70531a to 9741852 Compare July 2, 2018 22:37
@philipfweiss philipfweiss changed the base branch from pw--rheostat-add-css-building-scripts to master July 2, 2018 22:39
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from 9741852 to 926dc37 Compare July 2, 2018 22:42
Copy link
Collaborator

@majapw majapw left a 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",
Copy link
Collaborator

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

@@ -60,7 +60,7 @@ export default withStyles(({ color, unit }) => ({
},

DefaultProgressBar_progressBar: {
backgroundColor: color.teal,
backgroundColor: color.focus,
Copy link
Collaborator

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.

color: {
black: 'black',
white: '#fcfcfc',
grey: '#d8d8d8',
teal: '#abc4e8',
focus: '#abc4e8',
Copy link
Collaborator

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.

@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from 926dc37 to d39970f Compare July 2, 2018 22:59
@philipfweiss philipfweiss force-pushed the pw--clean-up-rheostat-CSS-theming branch from d39970f to 8eaa55e Compare July 3, 2018 00:06
@philipfweiss philipfweiss merged commit f54c6b5 into master Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants