Button - allow caller to specify icon for kind in theme and enhance kind padding #6297

taysea · 2022-08-23T22:51:53Z

What does this PR do?

Adds theme.button[kind].icon, theme.button.hover[kind].icon, theme.button.active[kind].icon, theme.button.disabled[kind].icon to allow theme to include a specific icon in a button kind.

Additionally, enhances theme.button.size to accept kind specific padding. This allows certain button kinds to take on different padding than other button kinds.

Also, in addition to adding relevant tests for this functionality, I fixed a size test that was written incorrectly. The snapshot was not showing the desired styling --> this PR updates the test and I have verified the snapshot changes now render the desired styling.

Also, adding string as an accepted button size -- based on need from upcoming grommet-theme-hpe changes.

Where should the reviewer start?

What testing has been done on this PR?

Locally in storybook by adjusted Themed Button TS story:

import React from 'react';

import { Box, Button, Grommet, Heading, Paragraph, Text } from 'grommet';
import { Hpe } from 'grommet-icons';
import { hpe } from 'grommet-theme-hpe';
import { deepMerge } from '../../../../utils';

const kindButtonTheme = deepMerge(hpe, {
  button: {
    size: {
      small: {
        border: {
          radius: '4px',
        },
        pad: {
          vertical: '4px',
          horizontal: '8px',
        },
        'cta-primary': {
          pad: {
            vertical: '3px',
            horizontal: '12px',
          },
        },
        'cta-alternate': {
          pad: {
            vertical: '3px',
            horizontal: '12px',
          },
        },
      },
      medium: {
        border: {
          radius: '4px',
        },
        pad: {
          vertical: '6px',
          horizontal: '12px',
        },
        'cta-primary': {
          pad: {
            vertical: '6px',
            horizontal: '16px',
          },
        },
        'cta-alternate': {
          pad: {
            vertical: '6px',
            horizontal: '16px',
          },
        },
      },
      large: {
        border: {
          radius: '6px',
        },
        pad: {
          vertical: '8px',
          horizontal: '16px',
        },
        'cta-primary': {
          pad: {
            vertical: '8px',
            horizontal: '20px',
          },
        },
        'cta-alternate': {
          pad: {
            vertical: '8px',
            horizontal: '20px',
          },
        },
      },
      xlarge: {
        border: {
          radius: '6px',
        },
        pad: {
          vertical: '10px',
          horizontal: '20px',
        },
        'cta-primary': {
          pad: {
            vertical: '10px',
            horizontal: '28px',
          },
        },
        'cta-alternate': {
          pad: {
            vertical: '10px',
            horizontal: '28px',
          },
        },
      },
    },
    extend: ({ sizeProp }) => {
      let fontSize = '19px';
      let lineHeight = '24px';
      if (sizeProp === 'small') {
        fontSize = '16px';
        lineHeight = '22px';
      } else if (sizeProp === 'large') {
        fontSize = '20px';
        lineHeight = '26px';
      } else if (sizeProp === 'xlarge') {
        fontSize = '22px';
        lineHeight = '28px';
      }
      return `
    font-size: ${fontSize};
    line-height: ${lineHeight}
    `;
    },
    'cta-primary': {
      background: { color: 'brand' },
      border: {
        radius: '100px',
      },
      color: 'white',
      font: { weight: 'bold' },
      icon: <Hpe />,
      reverse: true,
      size: {
        small: {
          pad: {
            vertical: '3px',
            horizontal: '12px',
          },
        },
      },
    },
    'cta-alternate': {
      background: 'background-contrast',
      border: {
        radius: '100px',
      },
      color: 'text-strong',
      font: {
        weight: 'bold',
      },
      icon: <Hpe color="brand" />,
      reverse: true,
    },
    active: {
      'cta-alternate': {
        icon: <Hpe color="brand" />,
      },
    },
    disabled: {
      'cta-alternate': {
        border: {
          width: '2px',
          color: 'text-xweak',
        },
      },
      'cta-primary': {
        border: {
          width: '2px',
          color: 'text-xweak',
        },
      },
    },
    secondary: {
      border: {
        color: 'brand',
      },
    },
  },
});

const customTheme = {
  global: {
    font: {
      family: 'Arial',
    },
  },
  button: {
    border: {
      radius: undefined,
      color: '#2196f3',
    },
    disabled: {
      color: 'orange',
      border: {
        color: 'orange',
      },
      extend: `border: 10px dashed red;`,
    },
    padding: {
      vertical: '12px',
      horizontal: '24px',
    },
    primary: {
      color: '#2196f3',
      active: {
        border: {
          color: 'red',
        },
        extend: `background: cadetblue;`,
      },
      extend: `background: skyblue; border: 5px dotted green;`,
    },
    extend: (props) => {
      let extraStyles = '';
      if (props.primary) {
        extraStyles = `
            text-transform: uppercase;
          `;
      }
      return `
          font-size: 12px;
          font-weight: bold;
          ${extraStyles}
        `;
    },
  },
};

const coloredButton = {
  button: {
    border: {
      color: 'accent-1',
    },
    color: { dark: 'accent-1', light: 'dark-2' },
    primary: {
      color: 'neutral-2',
    },
  },
};

const PrimaryButtons = () => (
  <Box gap="medium">
    <Button
      alignSelf="start"
      label="Primary small"
      onClick={() => {}}
      color="brand"
      primary
      size="small"
    />
    <Button
      alignSelf="start"
      label="Primary"
      onClick={() => {}}
      primary
      color="brand"
    />
    <Button
      alignSelf="start"
      label="Primary large"
      onClick={() => {}}
      primary
      color="brand"
      size="large"
    />
    <Button
      alignSelf="start"
      label="Primary xlarge"
      onClick={() => {}}
      primary
      color="brand"
      size="xlarge"
    />
    <Button
      alignSelf="start"
      label="Disabled Primary"
      onClick={() => {}}
      primary
      disabled
    />
    <Button
      alignSelf="start"
      label="Active Primary"
      onClick={() => {}}
      primary
      active
    />
  </Box>
);

const DefaultButtons = () => (
  <Box gap="medium">
    <Button
      alignSelf="start"
      label="Default small"
      onClick={() => {}}
      size="small"
    />
    <Button alignSelf="start" label="Default" onClick={() => {}} />
    <Button
      alignSelf="start"
      label="Default large"
      onClick={() => {}}
      size="large"
    />
    <Button label="Default xlarge" onClick={() => {}} size="xlarge" />
    <Button
      alignSelf="start"
      label="Disabled Default"
      onClick={() => {}}
      disabled
    />
    <Button
      alignSelf="start"
      label="Active Default"
      onClick={() => {}}
      active
    />
  </Box>
);

const SecondaryButtons = () => (
  <Box gap="medium">
    <Button
      alignSelf="start"
      label="Secondary small"
      onClick={() => {}}
      secondary
      size="small"
    />
    <Button alignSelf="start" label="Secondary" onClick={() => {}} secondary />
    <Button
      alignSelf="start"
      label="Secondary large"
      onClick={() => {}}
      size="large"
      secondary
    />
    <Button
      label="Secondary xlarge"
      onClick={() => {}}
      size="xlarge"
      secondary
    />
    <Button
      alignSelf="start"
      label="Disabled Secondary"
      onClick={() => {}}
      disabled
      secondary
    />
    <Button
      alignSelf="start"
      label="Active Secondary"
      onClick={() => {}}
      active
      secondary
    />
  </Box>
);

const CTAPrimary = () => (
  <Box gap="medium">
    <Button
      alignSelf="start"
      label="CTA Primary small"
      onClick={() => {}}
      kind="cta-primary"
      size="small"
    />
    <Button
      alignSelf="start"
      label="CTA Primary"
      onClick={() => {}}
      kind="cta-primary"
    />
    <Button
      alignSelf="start"
      label="CTA Primary large"
      onClick={() => {}}
      size="large"
      kind="cta-primary"
    />
    <Button
      label="CTA Primary xlarge"
      onClick={() => {}}
      size="xlarge"
      kind="cta-primary"
    />
    <Button
      alignSelf="start"
      label="Disabled CTA Primary"
      kind="cta-primary"
      onClick={() => {}}
      disabled
    />
    <Button
      alignSelf="start"
      label="Active CTA Primary"
      kind="cta-primary"
      onClick={() => {}}
      active
    />
  </Box>
);

const CTAAlternate = () => (
  <Box gap="medium">
    <Button
      alignSelf="start"
      label="CTA Alternate small"
      onClick={() => {}}
      size="small"
      kind="cta-alternate"
    />
    <Button
      alignSelf="start"
      label="CTA Alternate"
      onClick={() => {}}
      kind="cta-alternate"
    />
    <Button
      alignSelf="start"
      label="CTA Alternate large"
      onClick={() => {}}
      size="large"
      kind="cta-alternate"
    />
    <Button
      label="CTA Alternate xlarge"
      onClick={() => {}}
      size="xlarge"
      kind="cta-alternate"
    />
    <Button
      alignSelf="start"
      label="Disabled CTA Alternate"
      kind="cta-alternate"
      onClick={() => {}}
      disabled
    />
    <Button
      alignSelf="start"
      label="Active CTA Alternate"
      kind="cta-alternate"
      onClick={() => {}}
      active
    />
  </Box>
);

export const ThemedButtons = () => (
    <Grommet theme={kindButtonTheme}>
        <Box direction="row" gap="large">
          <DefaultButtons />
          <SecondaryButtons />
          <PrimaryButtons />
          <CTAPrimary />
          <CTAAlternate />
        </Box>
    </Grommet>
);

ThemedButtons.storyName = 'Themed Buttons';

export default {
  title: 'Controls/Button/Custom Themed/Themed Buttons',
};

How should this be manually tested?

In storybook. Can

Do Jest tests follow these best practices?

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Closes #6271

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes.

Should this PR be mentioned in the release notes?

Yes.

Is this change backwards compatible or is it a breaking change?

Backwards compatible.

MikeKingdom

Looks good. Just a minor typo in a comment

src/js/components/Button/Button.js

halocline

It seems odd to me that we would need to set if I don't want to modify it from its base:

    active: {
      'cta-alternate': {
        icon: <Hpe color="brand" />,
      },
    },

My expectation would be that if this is undefined, the active state would inherit the kind's base icon color.

Thoughts? Investigate getIconColor logic?

taysea · 2022-08-25T20:25:59Z

It seems odd to me that we would need to set if I don't want to modify it from its base:
    active: {
      'cta-alternate': {
        icon: <Hpe color="brand" />,
      },
    },
My expectation would be that if this is undefined, the active state would inherit the kind's base icon color.

Thoughts? Investigate getIconColor logic?

I would agree if it doesn't deviate from its base then I don't think we should have to define it again. Let me investigate the getIconColor logic.

EDIT: This should be resolved in the latest commit.

src/js/components/Button/Button.js

halocline

Functionality and implementation are looking good. Small types / theme question.

src/js/components/Button/index.d.ts

src/js/components/Button/propTypes.js

halocline · 2022-08-25T23:45:21Z

src/js/components/Button/propTypes.js

@@ -68,7 +68,7 @@ if (process.env.NODE_ENV !== 'production') {
    primary: PropTypes.bool,
    reverse: PropTypes.bool,
    secondary: PropTypes.bool,
-    size: PropTypes.oneOf(['small', 'medium', 'large']),
+    size: PropTypes.oneOf(['small', 'medium', 'large', 'xlarge']),


Also noticing, that kind is not in propTypes definition, nor is it documented. Is that still purposeful?

cc: @jcfilben

We do have it in typescript, so it feels like adding it here would be reasonable but maybe for sake of progressing this PR we can handle that in a small standalone PR (since it's an existing thing)/

Yes, I was not intending it for this PR, rather a point of awareness and stimulate discussion.

Also noticing, that kind is not in propTypes definition, nor is it documented. Is that still purposeful?

Sorry for slow response, I must have missed this comment when I was originally tagged. Anyway I'm looking into this... It also looks like the TS definition for kind is 'string' but looking at the code I think it could be a 'string' or 'object'. I'm not sure why we aren't documenting this, I can check with Shimi and maybe get more context.

src/js/components/Button/Button.js

src/js/components/Button/StyledButtonKind.js

Co-authored-by: Eric Soderberg <eric.soderberg@hpe.com>

Co-authored-by: Matthew Glissmann <mdglissmann@gmail.com>

…to 6271-kind-icon

halocline

Thank you, @taysea.

Allow caller to specify icon for Button kind in theme

2b464f2

taysea requested a review from halocline August 23, 2022 22:52

taysea mentioned this pull request Aug 23, 2022

Adjust button color and font-size/padding grommet/grommet-theme-hpe#271

Closed

halocline requested a review from MikeKingdom August 24, 2022 18:10

MikeKingdom reviewed Aug 24, 2022

View reviewed changes

src/js/components/Button/Button.js Outdated Show resolved Hide resolved

Update comment to fix typo but keep one line

3881114

taysea requested review from MikeKingdom and ericsoderberghp August 24, 2022 20:31

Merge branch 'master' into 6271-kind-icon

96971fa

MikeKingdom approved these changes Aug 24, 2022

View reviewed changes

halocline reviewed Aug 24, 2022

View reviewed changes

src/js/components/Button/Button.js Show resolved Hide resolved

Allow icon to change on hover

8f20eb1

taysea requested a review from halocline August 24, 2022 23:14

halocline reviewed Aug 25, 2022

View reviewed changes

Allow icon to reference base color

2d6260d

halocline reviewed Aug 25, 2022

View reviewed changes

src/js/components/Button/Button.js Show resolved Hide resolved

taysea added 2 commits August 25, 2022 15:13

Merge branch 'master' into 6271-kind-icon

0764aa9

Update bundleSize

89bb40f

taysea requested a review from halocline August 25, 2022 23:25

halocline reviewed Aug 25, 2022

View reviewed changes

ericsoderberghp reviewed Aug 25, 2022

View reviewed changes

taysea and others added 6 commits August 25, 2022 19:00

Update src/js/components/Button/Button.js

059c76e

Co-authored-by: Eric Soderberg <eric.soderberg@hpe.com>

Update src/js/components/Button/index.d.ts

73d8ac6

Co-authored-by: Matthew Glissmann <mdglissmann@gmail.com>

Update src/js/components/Button/propTypes.js

6ab2e9c

Co-authored-by: Matthew Glissmann <mdglissmann@gmail.com>

Incorporate feedback, simplify logic

7b14e3e

Remove empty line

bf47164

Merge branch '6271-kind-icon' of https://github.com/taysea/grommet in…

a3f88af

…to 6271-kind-icon

taysea requested a review from halocline August 26, 2022 17:37

halocline approved these changes Aug 26, 2022

View reviewed changes

taysea requested a review from ericsoderberghp August 26, 2022 18:05

ericsoderberghp approved these changes Aug 26, 2022

View reviewed changes

ericsoderberghp merged commit 6961c7e into grommet:master Aug 26, 2022

britt6612 mentioned this pull request Aug 30, 2022

Update docs grommet/grommet-site#412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Button - allow caller to specify icon for kind in theme and enhance kind padding #6297

Button - allow caller to specify icon for kind in theme and enhance kind padding #6297

Uh oh!

taysea commented Aug 23, 2022 •

edited

Loading

Uh oh!

MikeKingdom left a comment

Uh oh!

Uh oh!

Uh oh!

halocline left a comment •

edited

Loading

Uh oh!

taysea commented Aug 25, 2022 •

edited

Loading

Uh oh!

Uh oh!

halocline left a comment

Uh oh!

Uh oh!

Uh oh!

halocline Aug 25, 2022

Uh oh!

taysea Aug 26, 2022

Uh oh!

halocline Aug 26, 2022

Uh oh!

jcfilben Aug 30, 2022 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halocline left a comment

Uh oh!

Uh oh!

Button - allow caller to specify icon for kind in theme and enhance kind padding #6297

Button - allow caller to specify icon for kind in theme and enhance kind padding #6297

Uh oh!

Conversation

taysea commented Aug 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

Uh oh!

MikeKingdom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

halocline left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taysea commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

halocline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

halocline Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

taysea Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

halocline Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

jcfilben Aug 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halocline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

taysea commented Aug 23, 2022 •

edited

Loading

halocline left a comment •

edited

Loading

taysea commented Aug 25, 2022 •

edited

Loading

jcfilben Aug 30, 2022 •

edited

Loading