Skip to content

Add theme.icon.matchSize #6653

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

Merged
merged 6 commits into from
Mar 2, 2023
Merged

Add theme.icon.matchSize #6653

merged 6 commits into from
Mar 2, 2023

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented Feb 27, 2023

What does this PR do?

Adds theme.icon.matchSize as a boolean in the theme. When this is true, components with icon and size will pass the size onto the icon.

This still allows for any custom overrides via size prop on icon to come through.

For TextInput and MaskedInput, I had to adjust how the padding was calculating to account for the variable icon size. I ensured that there was a 12px gap between icon and placeholder text/value across all sizes.

Where should the reviewer start?

Any component.

What testing has been done on this PR?

Tested in storybook (see screenshot) and added jest tests.

How should this be manually tested?

import React from 'react';
import {
  Anchor,
  Grommet,
  Button,
  Box,
  Menu,
  Pagination,
  Select,
  SelectMultiple,
  MaskedInput,
  TextInput,
  DateInput,
  Text,
} from 'grommet';
import { LinkNext, Down, Search } from 'grommet-icons';

export const Active = () => (
  <Grommet
    theme={{
      global: {
        font: {
          family: `-apple-system, BlinkMacSystemFont`,
        },
      },
      icon: {
        matchSize: true,
        size: {
          small: '12px',
          medium: '18px',
          large: '24px',
        },
      },
      button: {
        default: {
          background: undefined,
          border: {
            radius: '2em',
          },
          pad: {
            horizontal: '12px',
            vertical: '12px',
          },
        },
        primary: {
          background: 'brand',
          border: {
            radius: '2em',
          },
          pad: {
            horizontal: '12px',
            vertical: '12px',
          },
        },
        size: {
          // small: {
          //   pad: '9px',
          // },
          // large: {
          //   pad: '9px',
          // },
        },
      }, // enabling kind button functionality
      menu: {
        icons: {
          down: Down,
        },
      },
      select: {
        icons: {
          down: Down,
        },
      },
    }}
  >
    <Box pad="large" align="center" gap="medium">
      <Button icon={<LinkNext />} size="small" primary />
      <Button icon={<LinkNext />} primary />
      <Button icon={<LinkNext />} size="large" primary />
      <Button
        icon={<LinkNext size="12px" />}
        label="Explicit size override on icon"
        size="large"
        primary
        reverse
      />
    </Box>
    <Box pad="large" align="center" gap="medium">
      <Pagination numberItems={200} size="small" />
      <Pagination numberItems={200} />
      <Pagination numberItems={200} size="large" />
    </Box>
    <Box pad="large" align="center" gap="medium">
      <Menu label="Menu" size="small" />
      <Menu label="Menu" />
      <Menu label="Menu" size="large" />
    </Box>
    <Box pad="large" align="center" gap="medium">
      <Select placeholder="Select" size="small" options={[]} />
      <Select placeholder="Select" options={[]} />
      <Select placeholder="Select" size="large" options={[]} />
    </Box>
    <Box pad="large" align="center" gap="medium">
      <SelectMultiple placeholder="SelectMultiple" size="small" options={[]} />
      <SelectMultiple placeholder="SelectMultiple" options={[]} />
      <SelectMultiple placeholder="SelectMultiple" size="large" options={[]} />
    </Box>
    <Box pad="large" align="center" gap="medium" width="medium">
      <TextInput placeholder="TextInput" size="small" icon={<Search />} />
      <TextInput placeholder="TextInput" icon={<Search />} />
      <TextInput placeholder="TextInput" size="large" icon={<Search />} />
    </Box>
    <Box pad="large" align="center" gap="medium" width="medium">
      <MaskedInput placeholder="MaskedInput" size="small" icon={<Search />} />
      <MaskedInput placeholder="MaskedInput" icon={<Search />} />
      <MaskedInput placeholder="MaskedInput" size="large" icon={<Search />} />
    </Box>
    <Box pad="large" align="center" gap="medium" width="medium">
      <DateInput format="mm/dd/yyyy" size="small" />
      <DateInput format="mm/dd/yyyy" />
      <DateInput format="mm/dd/yyyy" size="large" />
    </Box>
    <Box pad="large" align="center" gap="medium" width="medium">
      <Text size="small">
        This is <Anchor label="small" icon={<LinkNext />} reverse />
      </Text>
      <Text>
        This is <Anchor label="medium" icon={<LinkNext />} reverse />
      </Text>
      <Text size="large">
        This is <Anchor label="large" icon={<LinkNext />} reverse />
      </Text>
    </Box>
  </Grommet>
);

Active.storyName = 'Active';

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

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?

Screenshots (if appropriate)

localhost_9001_iframe html_globals=theme_base args= id=controls-button-custom-themed-active--active viewMode=story

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.

@@ -0,0 +1,13 @@
import { cloneElement } from 'react';

export const useSizedIcon = (iconArg, size, theme) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it too confusing to do:

export const useSizedIcon = (icon, size, theme) =>
  (icon && theme?.icon?.matchSize && !icon.props.size && cloneElement(icon, { size })) || undefined;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like it -- pushing a change that is along these lines with slight tweak to logic. (need to return icon as is if the conditions return false)

@taysea taysea requested a review from ericsoderberghp March 2, 2023 19:15
@taysea taysea merged commit c20b405 into grommet:master Mar 2, 2023
@taysea taysea deleted the icon-matchSize branch March 2, 2023 19:33
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.

2 participants