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

Add size prop to Icon component #796

Merged
merged 10 commits into from
Oct 25, 2024
Merged

Conversation

joshfarrant
Copy link
Contributor

@joshfarrant joshfarrant commented Oct 23, 2024

Summary

Adds size prop to Icon component following @simmonsjenna's suggested sizing, and @danielguillan's Figma designs.

List of notable changes:

  • Added size prop
  • Added tests
  • Updates stories
  • Condensed styles

What should reviewers focus on?

  • There's an !important in the CSS. If you're not happy with that then I'm happy to tweak the approach to use specificity to override the background styles instead.
  • Happy with the new stories?

Steps to test:

  • Play around in the Storybook playground

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

image

Copy link

changeset-bot bot commented Oct 23, 2024

🦋 Changeset detected

Latest commit: 2fef888

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/react-brand Patch
@primer/brand-primitives Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@joshfarrant joshfarrant force-pushed the joshfarrant/integrate-icon-component branch from a93f66e to 158f186 Compare October 23, 2024 12:57
Copy link
Contributor

github-actions bot commented Oct 23, 2024

🟢 No design token changes found

Copy link
Contributor

github-actions bot commented Oct 23, 2024

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

artifacts section of workflow run

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

@joshfarrant
Copy link
Contributor Author

NextJS seem to be in the process of messing around with their React version (they're currently on a React 19 RC), which is the underlying cause of the pipeline failure.

If you're happy to just ignore the failing check for now @rezrah then we can come back to it once it's settled down.

export const namedIconSizes = ['small', 'medium', 'large'] as const
export type NamedIconSize = (typeof namedIconSizes)[number]

export const numericIconSizes = [12, 16, 20, 24, 28, 32, 36, 40, 44, 48, 52] as const
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we max out at 44 please per this guidance

Suggested change
export const numericIconSizes = [12, 16, 20, 24, 28, 32, 36, 40, 44, 48, 52] as const
export const numericIconSizes = [20, 24, 28, 32, 36, 40, 44] as const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do that if @danielguillan is happy.

These numbers have come from Dani's FIgma designs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah cool, that's good to know. We'll need to follow up In Figma after too, as the Site guidance supersedes that. Particularly in the React version, we'd want to provide the correct guardrails so recommend we go ahead with the 20-44 range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's updated

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to limit the scale and expand it later if needed. I just want to check with @simmonsjenna to confirm that we've considered scenarios beyond Bento, Card, and Pillar in making this decision. For example, I'm thinking about icons that align with the default body text (16px) in Primer Brand components like Button, Label, and UnorderedList, among others. I'm not suggesting we necessarily use Icon in those, but those are potential use cases. Also custom elements might benefit from the expanded scale as there are several places in dotcom where use icons smaller than 20 and larger than 44.

Copy link

@simmonsjenna simmonsjenna Oct 25, 2024

Choose a reason for hiding this comment

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

@danielguillan Great point — I need to do a more in-depth audit of icon usage across those scenarios you mentioned. I think we can expand it later to include those smaller icon instances as well.

Are there any examples where an icon larger than 44 is live?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

I believe there's only one icon larger than 44 in landing pages but there are some more on product pages that could potentially use Primer Brand components at some point (e.g., upsell). That said, we don't need to support those larger sizes if we want to discourage that. Octicons aren't actually designed for larger sizes either way. We can create some guardrails around that.

Copy link
Collaborator

@rezrah rezrah left a comment

Choose a reason for hiding this comment

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

Looks great! Visual snapshots will need an update, but otherwise :shipit:

Copy link
Contributor

@danielguillan danielguillan left a comment

Choose a reason for hiding this comment

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

Looks great! I left one comment about the numeric size scale, but it's non-blocking.

@joshfarrant joshfarrant merged commit 3104149 into main Oct 25, 2024
2 checks passed
@joshfarrant joshfarrant deleted the joshfarrant/integrate-icon-component branch October 25, 2024 08:22
@primer-css primer-css mentioned this pull request Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants