feat: Checkbox color updates & Extending theme based styling to variants #46

jadenlemmon · 2019-07-12T20:55:57Z

Summary

Add the ability to pass colorOn & colorOff when using a CheckboxGroup
Add the ability to extend theme based styling into variants as well
Changes are based on issues encountered with PRD updates

Theme styling updates

Problem
When extending the styles for a particular component at the theme level, I can't dig deeper into a per variant basis. This causes problems when trying to change the hover state for a secondary button only for example.

Solution
Allow for the ability to pass styles into the variant level as well.

For example if I want to target a secondary button only I can pass this following in theme.js. The style key accepts a template string or a style object.

{  
  Button: {
      variants: {
        secondary: {
          style: `
            &:hover {
              background-color: tomato;
            }
          `,
        },
      },
    },
}

codecov · 2019-07-12T20:56:02Z

Codecov Report

Merging #46 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   68.36%   68.57%   +0.21%     
==========================================
  Files          22       22              
  Lines         433      436       +3     
  Branches       92       92              
==========================================
+ Hits          296      299       +3     
  Misses        107      107              
  Partials       30       30

Impacted Files	Coverage Δ
src/utils.js	`95.12% <100%> (+0.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fada878...fa2cd3b. Read the comment docs.

codecov · 2019-07-12T20:56:03Z

Codecov Report

Merging #46 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   68.36%   68.57%   +0.21%     
==========================================
  Files          22       22              
  Lines         433      436       +3     
  Branches       92       92              
==========================================
+ Hits          296      299       +3     
  Misses        107      107              
  Partials       30       30

Impacted Files	Coverage Δ
src/utils.js	`95.12% <100%> (+0.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fada878...fa2cd3b. Read the comment docs.

codecov · 2019-07-12T20:56:03Z

Codecov Report

Merging #46 into master will increase coverage by 0.21%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   68.36%   68.57%   +0.21%     
==========================================
  Files          22       22              
  Lines         433      436       +3     
  Branches       92       92              
==========================================
+ Hits          296      299       +3     
  Misses        107      107              
  Partials       30       30

Impacted Files	Coverage Δ
src/utils.js	`95.12% <100%> (+0.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fada878...fa2cd3b. Read the comment docs.

codecov · 2019-07-12T20:56:03Z

Codecov Report

Merging #46 into master will increase coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #46      +/-   ##
=========================================
+ Coverage   68.36%   68.5%   +0.14%     
=========================================
  Files          22      22              
  Lines         433     435       +2     
  Branches       92      92              
=========================================
+ Hits          296     298       +2     
  Misses        107     107              
  Partials       30      30

Impacted Files	Coverage Δ
src/utils.js	`95% <100%> (+0.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fada878...e8bde4e. Read the comment docs.

erikshestopal · 2019-07-12T22:22:15Z

src/utils.js

  if (!config) throw new Error(`Molekule: "${variant}" variant not found in theme...`);
  return config;
 };

 export const getComponentStyle = componentName => themeGet(`components.${componentName}.style`);

+export const getComponentVariantStyles = (componentName, variant) =>
+  themeGet(`components.${componentName}.variants.${variant}.style`);


@jlemm45 This will return a function instead of a value because themeGet returns a function that accepts theme as an argument.

This seems to be the way it has been used here https://github.com/sappira-inc/molekule/blob/0656ca13a18efe5b70ebbced48f3848572fe321f/src/utils.js#L15 ? LMK @erikshestopal

cehsu · 2019-07-12T23:38:49Z

src/utils.js

@@ -4,15 +4,19 @@ import styled from 'styled-components';
 export const themeGet = (lookup, fallback) => ({ theme } = {}) => get(theme, lookup, fallback);

 export const getComponentVariant = (theme, componentName, variant) => {
-  const config = themeGet(`components.${componentName}.variants.${variant}`, theme.variants[componentName][variant])({
+  const config = themeGet(`variants.${componentName}.${variant}`, theme.variants[componentName][variant])({


We are now using the same thing for the lookup and the fallback? Should we keep the other property (components.${componentName}.variants.${variant}) as the fallback now? If not, we might want to bump the version number, since it's potentially a breaking change.

@cehsu I bumped the major version

cehsu · 2019-07-12T23:53:02Z

src/utils.js

  if (!config) throw new Error(`Molekule: "${variant}" variant not found in theme...`);
  return config;
 };

 export const getComponentStyle = componentName => themeGet(`components.${componentName}.style`);

+export const getComponentVariantStyles = (componentName, variant) =>
+  themeGet(`components.${componentName}.variants.${variant}.style`);


It looks like we want to support two conventions: components.${componentName}.variants.${variant} and also variants.${componentName}.${variant} (similar to getComponentVariant)?

I believe I addressed this here https://github.com/sappira-inc/molekule/blob/0656ca13a18efe5b70ebbced48f3848572fe321f/src/utils.js#L20 LMK @cehsu

cehsu

Looks good to me. I'll do some QA, and plan to approve.

kylealwyn

Component variants are already defined in isolation, .e.g https://github.com/heydoctor/molekule/blob/master/src/theme.js#L148-L152. It may make more sense to add the style property to the variant declaration:

const buttonVariants = {
  primary: {
    backgroundColor: colors.primary,
    fontColor: 'white',
    style: css`
	  &:hover {
         background: coral;
      }
	`
  },
}

// 
const { backgroundColor, fontColor, style } = getComponentVariant(theme, 'Button', variant);

return css`
	${style}
`

In getComponentVariant, we can merge our custom variant with the default variant. We can discuss further at all hands

package.json

This reverts commit fba7a2b.

jadenlemmon · 2019-07-16T18:45:33Z

@kylealwyn Per @cehsu's comments I've already included that pattern you mention in tandem with the pattern above so you can do the following.

{
  Button: {
      variants: {
        secondary: {
          style: `
            &:hover {
              background-color: tomato;
            }
          `,
        },
      },
  }
}

or what you mention above

{
  primary: {
    backgroundColor: colors.primary,
    fontColor: 'white',
    style: css`
	  &:hover {
         background: coral;
      },
  },
}

kylealwyn · 2019-07-16T18:49:08Z

@jlemm45 @cehsu I don't see why we would need or want to support both methods of styling variants

jadenlemmon · 2019-07-16T18:53:56Z

@kylealwyn @cehsu Happy to remove the first pattern if no one sees a need.

kylealwyn · 2019-07-16T18:56:09Z

@jlemm45 Lets remove

kylealwyn · 2019-07-16T19:08:06Z

@heydoctor/engineering Given this is already going going to be a breaking change, albeit a minor, one file break, I propose we convert component style overrides to the same pattern as variants:

const theme = {
	...,
    variants: {
        Button: {
          primary: { 
            style: css`
              background: coral;
            `
          }
		},
    },
    styles: {
        Button: `
          border-radius: 42px;
        `
     }
}

cehsu

Looks good to me, should we add some tests for these utils?

cehsu · 2019-07-17T05:25:52Z

src/Form/CheckboxGroup.js

@@ -77,7 +78,8 @@ class CheckboxGroup extends Component {
                  id={key}
                  name={key}
                  label={choice.label}
-                  color={color}
+                  colorOn={colorOn}
+                  colorOff={colorOff}


jadenlemmon · 2019-07-17T20:52:20Z

@cehsu Are referring to testing the new util functions in utils.js? Just want to confirm.

jadenlemmon · 2019-07-17T22:37:31Z

@cehsu I added some tests. LMK if you were thinking of something different. @kylealwyn Any further thoughts?

kylealwyn

Awesome

jadenlemmon added 2 commits July 11, 2019 22:26

Allow for color props for CheckboxGroup

1da7194

Modify to support better nested styling

fa2cd3b

jadenlemmon requested a review from a team July 12, 2019 20:55

ghost requested review from cehsu and kylealwyn July 12, 2019 20:56

erikshestopal reviewed Jul 12, 2019

View reviewed changes

cehsu reviewed Jul 12, 2019

View reviewed changes

jadenlemmon added 2 commits July 15, 2019 16:39

Update to support another style syntax, Address feedback

0656ca1

Major version bump

fba7a2b

cehsu reviewed Jul 16, 2019

View reviewed changes

kylealwyn suggested changes Jul 16, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

Revert "Major version bump"

a54bb40

This reverts commit fba7a2b.

kylealwyn changed the title ~~Checkbox color updates & Extending theme based styling to variants~~ feat: Checkbox color updates & Extending theme based styling to variants Jul 16, 2019

jadenlemmon added 2 commits July 16, 2019 16:00

Remove extra pattern

586a045

Add toplevel styles instead of components

9e2a9cb

cehsu approved these changes Jul 17, 2019

View reviewed changes

Add util tests

e8bde4e

kylealwyn approved these changes Jul 17, 2019

View reviewed changes

jadenlemmon merged commit 6f3569f into master Jul 18, 2019

jadenlemmon deleted the feat/patient-rd-color-scheme branch July 18, 2019 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Checkbox color updates & Extending theme based styling to variants #46

feat: Checkbox color updates & Extending theme based styling to variants #46

jadenlemmon commented Jul 12, 2019

codecov bot commented Jul 12, 2019

codecov bot commented Jul 12, 2019

codecov bot commented Jul 12, 2019

codecov bot commented Jul 12, 2019 •

edited

Loading

erikshestopal Jul 12, 2019 •

edited

Loading

jadenlemmon Jul 15, 2019 •

edited

Loading

cehsu Jul 12, 2019 •

edited

Loading

jadenlemmon Jul 15, 2019

cehsu Jul 12, 2019

jadenlemmon Jul 15, 2019 •

edited

Loading

cehsu left a comment

kylealwyn left a comment

jadenlemmon commented Jul 16, 2019

kylealwyn commented Jul 16, 2019

jadenlemmon commented Jul 16, 2019

kylealwyn commented Jul 16, 2019

kylealwyn commented Jul 16, 2019 •

edited

Loading

cehsu left a comment

cehsu Jul 17, 2019

jadenlemmon commented Jul 17, 2019 •

edited

Loading

jadenlemmon commented Jul 17, 2019

kylealwyn left a comment

feat: Checkbox color updates & Extending theme based styling to variants #46

feat: Checkbox color updates & Extending theme based styling to variants #46

Conversation

jadenlemmon commented Jul 12, 2019

Summary

Theme styling updates

codecov bot commented Jul 12, 2019

Codecov Report

codecov bot commented Jul 12, 2019

Codecov Report

codecov bot commented Jul 12, 2019

Codecov Report

codecov bot commented Jul 12, 2019 • edited Loading

Codecov Report

erikshestopal Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

jadenlemmon Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

cehsu Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

jadenlemmon Jul 15, 2019

Choose a reason for hiding this comment

cehsu Jul 12, 2019

Choose a reason for hiding this comment

jadenlemmon Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

cehsu left a comment

Choose a reason for hiding this comment

kylealwyn left a comment

Choose a reason for hiding this comment

jadenlemmon commented Jul 16, 2019

kylealwyn commented Jul 16, 2019

jadenlemmon commented Jul 16, 2019

kylealwyn commented Jul 16, 2019

kylealwyn commented Jul 16, 2019 • edited Loading

cehsu left a comment

Choose a reason for hiding this comment

cehsu Jul 17, 2019

Choose a reason for hiding this comment

jadenlemmon commented Jul 17, 2019 • edited Loading

jadenlemmon commented Jul 17, 2019

kylealwyn left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 12, 2019 •

edited

Loading

erikshestopal Jul 12, 2019 •

edited

Loading

jadenlemmon Jul 15, 2019 •

edited

Loading

cehsu Jul 12, 2019 •

edited

Loading

jadenlemmon Jul 15, 2019 •

edited

Loading

kylealwyn commented Jul 16, 2019 •

edited

Loading

jadenlemmon commented Jul 17, 2019 •

edited

Loading