Skip to content

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented Jan 26, 2023

What does this PR do?

Adds support for custom padding for icon-only buttons (buttons that have icon and no label). Having the ability to specify icon only padding is desirable to create uniform dimension icon only buttons.

Additionally, it is desirable to be able to manage icon size from the theme and vary the icon's size depending on what size button it is contained in.

This PR adds:

  • theme.button.icon.size (allows caller to specify what size the icon should render as when contained in a small, medium, or large button
  icon?: {
    size?: {
      small?: string;
      medium?: string;
      large?: string;
    };
  };
  • theme.button.size[size].iconOnly.pad which allows the caller to specify pad for iconOnly pad across button sizes.

Where should the reviewer start?

base.js

What testing has been done on this PR?

Tested in local story:

Screen Shot 2023-01-26 at 2 44 10 PM

How should this be manually tested?

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?

Related to: grommet/hpe-design-system#3117

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.

@taysea taysea requested a review from halocline February 2, 2023 00:20
@ericsoderberghp ericsoderberghp dismissed their stale review February 2, 2023 01:16

noticed some issues remain

@ericsoderberghp
Copy link
Contributor

Chromatic picked up some things that look a bit off.

Pagination large vertical alignment:
Screenshot 2023-02-01 at 5 17 03 PM

Button large icon and padding don't look right here:
Screenshot 2023-02-01 at 5 18 18 PM

Custom themed Button changed a bit unexpectedly:
Screenshot 2023-02-01 at 5 19 23 PM

Screenshot 2023-02-01 at 5 20 12 PM

@taysea taysea marked this pull request as draft February 2, 2023 18:43
@taysea taysea marked this pull request as ready for review February 2, 2023 19:52
@taysea taysea marked this pull request as draft February 2, 2023 19:56
@taysea taysea marked this pull request as ready for review February 2, 2023 19:59
@ericsoderberghp ericsoderberghp merged commit 7b0d709 into grommet:master Feb 2, 2023
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