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.

Sign up for GitHub

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

Jump to bottom

Banner component #1036

Merged
merged 37 commits into from
Feb 15, 2023
Merged

Banner component #1036

merged 37 commits into from
Feb 15, 2023

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Feb 7, 2023

Pull Request

🤨 Rationale

See #305

👩‍💻 Implementation

  • Created new banner component (not based on FAST)
  • Added unit tests and visual Storybook tests
  • Updated README

🧪 Testing

Unit tests and Storybook testing

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc m-akinc changed the title Users/makinc/banner Banner component Feb 7, 2023
@m-akinc m-akinc requested a review from jattasNI February 8, 2023 16:55
@nate-ni nate-ni mentioned this pull request Feb 8, 2023
Closed
jattasNI
jattasNI reviewed Feb 9, 2023
View reviewed changes
Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

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

I'd like to take one more look at the storybook before approving but it looks like it didn't build with the current revision, possibly due to merge conflicts. Could you please reset me when it's available again?

@m-akinc m-akinc requested a review from rajsite February 10, 2023 17:01
@m-akinc @mollykreis
Update packages/nimble-components/src/banner/tests/banner.stories.ts
b1d6117 
Co-authored-by: mollykreis <20542556+mollykreis@users.noreply.github.com>
rajsite
rajsite approved these changes Feb 14, 2023
View reviewed changes
packages/nimble-components/src/banner/types.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/types.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/index.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/styles.ts Show resolved Hide resolved
packages/nimble-components/src/banner/styles.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/styles.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/styles.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/styles.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/template.ts Outdated Show resolved Hide resolved
packages/nimble-components/src/banner/tests/banner-matrix.stories.ts Outdated Show resolved Hide resolved
rajsite
rajsite requested changes Feb 15, 2023
View reviewed changes
Copy link
Member

@rajsite rajsite left a comment

Choose a reason for hiding this comment

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

Marking request changes to take a look after the title aria behavior updates

@m-akinc m-akinc requested a review from rajsite February 15, 2023 18:33
@rajsite rajsite merged commit 0248f3d into main Feb 15, 2023
@rajsite rajsite deleted the users/makinc/banner branch February 15, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mollykreis mollykreis mollykreis approved these changes

@rajsite rajsite rajsite approved these changes

@jattasNI jattasNI jattasNI approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@m-akinc @rajsite @jattasNI @mollykreis