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

feat(Banner): add banner component #4335

Merged
merged 50 commits into from
Apr 24, 2024
Merged

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Mar 1, 2024

Closes https://github.com/github/primer/issues/3200

Add support for the Banner component to Primer React.

Changelog

New

  • A new Banner component is available under experimental/drafts
  • Supporting stories, tests, and docs for Banner

Changed

Removed

Rollout strategy

  • Minor release

Testing & Reviewing

  • Verify implementation matches API design in API Document
  • What do you think of the API design? Is it flexible enough for us to iterate on? Does it have any gaps or are there opportunities we should consider?
  • Verify responsive behavior for buttons
  • Verify that all the limitations for banner are incorporated here and tested, for example when custom icons are allowed or when banners are allowed to be dismissible

Copy link

changeset-bot bot commented Mar 1, 2024

🦋 Changeset detected

Latest commit: fe5b3ed

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

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

Copy link
Contributor

github-actions bot commented Mar 1, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 88 KB (0%)
packages/react/dist/browser.umd.js 88.25 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-4335 March 1, 2024 20:46 Inactive
@lukasoppermann
Copy link
Contributor

Screenshot showing where to move the button

Another thing, whenever a title is present, the actions (not close button) should be moved below the text.

@lukasoppermann
Copy link
Contributor

Hey @joshblack,

this is really great already. Do you think we could add the in sidebar example to the playground as well?
https://primer-65defb5e3c-13348165.drafts.github.io/storybook/?path=/story/drafts-components-banner--playground

E.g. like this:

Mockup for both banner sizes next to each other

@lukasoppermann
Copy link
Contributor

When the title is disabled, there is a larger space below the actions in the banner than above.

space below buttons

@joshblack
Copy link
Member Author

Thanks @lukasoppermann! I believe I addressed the following:

  • When the title is disabled, there is a larger space below the actions in the banner than above.
  • Do you think we could add the in sidebar example to the playground as well?
  • Another thing, whenever a title is present, the actions (not close button) should be moved below the text.
  • It is 6px to short (in figma there is an inner padding in the action container when the buttons are below the content).

@lukasoppermann
Copy link
Contributor

@joshblack we should add a test to make sure svgs work to avoid this issue: #3020

@joshblack
Copy link
Member Author

@lukasoppermann we should be good to go thankfully! What we have right now will only be behind the .BannerIcon class name so should completely avoid the problem 🤞

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.

4 participants