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

[DTO-4855] info banner changes #3072

Merged
merged 43 commits into from
Nov 15, 2023
Merged

[DTO-4855] info banner changes #3072

merged 43 commits into from
Nov 15, 2023

Conversation

sambocharov
Copy link
Contributor

Changes:

  • BpkInfoBanner component created according to the figma design
  • Depricated BpkBannerAlert

New Version Screenshots
Screenshot 2023-11-07 at 15 10 49
Screenshot 2023-11-07 at 15 11 15

Deprication of BpkBannerAlert:
Please, replace BpkBannerAlert with BpkInfoBanner

Remember to include the following changes:

  • README.md (If you have created a new component)
  • Component README.md
  • Tests
  • Storybook examples created/updated
  • For breaking changes or deprecating components/properties, migration guides added to the description of the PR. If the guide has large changes, consider creating a new Markdown page inside the component's docs folder and link it here

@sambocharov sambocharov added the minor Non breaking change label Nov 7, 2023
examples/bpk-component-info-banner/examples.js Outdated Show resolved Hide resolved
examples/bpk-component-info-banner/examples.js Outdated Show resolved Hide resolved
examples/bpk-component-info-banner/examples.js Outdated Show resolved Hide resolved
examples/bpk-component-info-banner/examples.js Outdated Show resolved Hide resolved
examples/bpk-component-info-banner/examples.js Outdated Show resolved Hide resolved
packages/bpk-component-info-banner/src/themeAttributes.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Nov 8, 2023

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 3c51c4e

Copy link

github-actions bot commented Nov 8, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

4 similar comments
Copy link

github-actions bot commented Nov 8, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 8, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 8, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 8, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 8, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

5 similar comments
Copy link

github-actions bot commented Nov 9, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 9, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 9, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

github-actions bot commented Nov 9, 2023

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link
Member

@olliecurtis olliecurtis left a comment

Choose a reason for hiding this comment

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

Could you take a look at the default storybook example as it seems to be erroring when trying to access it: https://backpack.github.io/storybook-prs/3072/?path=/story/bpk-component-info-banner--default

Copy link
Member

Choose a reason for hiding this comment

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

One thing I notice missing here from the design and the API design is for the expandable is the action property that is optional and manages if to show an CTA inside and sets the click handler

Screenshot 2023-11-14 at 13 19 04

Copy link
Member

@olliecurtis olliecurtis left a comment

Choose a reason for hiding this comment

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

Sorry accidently clicked approve with the above request

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

1 similar comment
Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

@@ -167,6 +172,10 @@ const BpkInfoBannerInner = ({
isExpandable && 'bpk-info-banner__header--expandable'
);

const childrenContainerClassName = action && isExpandable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

had to do this conditional class to add different spacing. Will need to check with designers as well, since with "action" button implementation was not reviewed.

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link
Member

@olliecurtis olliecurtis left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the contribution and sticking by during this 😄

party time

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

Copy link

Visit https://backpack.github.io/storybook-prs/3072 to see this build running in a browser.

@olliecurtis olliecurtis merged commit 214a903 into main Nov 15, 2023
9 checks passed
@olliecurtis olliecurtis deleted the DTO-4855-info-banner-changes branch November 15, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Non breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants