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

[SAGE-517] Banner: Add storybook examples and update logic to pass active state through components #1554

Merged
merged 1 commit into from
Aug 16, 2022

Conversation

goodwinchris
Copy link
Contributor

@goodwinchris goodwinchris commented Aug 12, 2022

Description

Update banner logic slightly and add more stories to bring storybook into parity with rails examples.

Note: Original issue noted in ticket is no longer relevant

Screenshots

before after
CleanShot 2022-08-12 at 16 00 42@2x CleanShot 2022-08-12 at 16 00 31@2x

Testing in sage-lib

Open react app, check banner stories.
http://localhost:4100/?path=/docs/sage-banner--default

Testing in kajabi-products

  1. (LOW) Updated logic for banner to pass active state into context and wrapper components
    • Updated banner logic has been tested in storybook and confirmed through review and react component has minimal use

Related

https://kajabi.atlassian.net/browse/SAGE-517

@goodwinchris goodwinchris changed the title fix(banner story): add more info to banner story [SAGE-517] Alert: Small with no icon causes layout bug Aug 12, 2022
@goodwinchris goodwinchris self-assigned this Aug 12, 2022
@goodwinchris goodwinchris requested review from a team August 12, 2022 19:59
@goodwinchris goodwinchris marked this pull request as ready for review August 12, 2022 19:59
@goodwinchris
Copy link
Contributor Author

Question for reviewers: How would you rate this kind of change in terms of KP risk?

Copy link
Collaborator

@ju-Skinner ju-Skinner left a comment

Choose a reason for hiding this comment

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

LGTM...job well done

@ju-Skinner ju-Skinner requested a review from a team August 12, 2022 20:56
Copy link
Member

@teenwolfblitzer teenwolfblitzer left a comment

Choose a reason for hiding this comment

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

👍🏼 LGTM! The title/description of the PR was confusing to me, since it looks like the original ticket referenced the incorrect component "Alert" rather than "Banner". Perhaps the Jira ticket should be updated rather than the PR to make that clear?

bannerContext: 'sage-demo'
};

export const SecondaryInlineBanner = Template.bind({});
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@teenwolfblitzer
Copy link
Member

teenwolfblitzer commented Aug 15, 2022

Question for reviewers: How would you rate this kind of change in terms of KP risk?

There's about a dozen uses of it in the app to check, primarily centered around the Product creation wizard and trial/account limits. Consider rating as a MED or HIGH since some areas are high visibility.

EDIT: whoops, just realized I was still thinking Alert rather than Banner, which I don't believe has much use in React (mostly Rails)

);

Banner.TYPES = BANNER_TYPES;

// Defining all default props and prop types explicitly to populate story table
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 😍

Copy link
Contributor

@QuintonJason QuintonJason left a comment

Choose a reason for hiding this comment

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

Nice updates. I would also tag this documentation since you added more examples

@goodwinchris goodwinchris added the documentation Improvements or additions to documentation label Aug 16, 2022
@goodwinchris goodwinchris changed the title [SAGE-517] Alert: Small with no icon causes layout bug [SAGE-517] Banner: Add storybook examples and update logic to pass active state through components Aug 16, 2022
@goodwinchris goodwinchris merged commit df10f46 into develop Aug 16, 2022
@goodwinchris goodwinchris deleted the SAGE-517/cg-alert-story-fixes branch August 16, 2022 20:57
@goodwinchris goodwinchris mentioned this pull request Aug 16, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants