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

fix(alert): grid template areas #1741

Merged
merged 3 commits into from
May 16, 2023
Merged

Conversation

ju-Skinner
Copy link
Collaborator

@ju-Skinner ju-Skinner commented May 15, 2023

Jira Ticket: https://kajabi.atlassian.net/browse/DSS-405

Description

When the Alert has no actions, the additional row was still being displayed. This is not the intended behavior.

Screenshots

Before After
image
image

Testing in sage-lib

  1. Navigate to Storybook -> Alert
  2. Verify that there is no additional row added when the alert has no Actions.

Testing in kajabi-products

I need to find a location in the app that has an Alert with no action buttons. At this time I'm unaware of it. I'll try to update prior to merging.

@ju-Skinner ju-Skinner self-assigned this May 15, 2023
@ju-Skinner ju-Skinner changed the title Fix/alert grid template areas fix(alert): grid template areas May 15, 2023
@ju-Skinner ju-Skinner requested a review from a team May 15, 2023 16:00
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

Seeing some minor layout issues on the Rails side and the linter is yelling a bit. After those are addressed, it should be good to go.

packages/sage-react/lib/Alert/Alert.jsx Show resolved Hide resolved
@pixelflips pixelflips requested a review from a team May 15, 2023 18:03
@ju-Skinner ju-Skinner force-pushed the fix/alert-grid-template-areas branch from c2ddd19 to c5e72d6 Compare May 15, 2023 18:20
this introduced a default story with minimum data

this also adds stories with and without action buttons

fix: DSS-405
@ju-Skinner ju-Skinner force-pushed the fix/alert-grid-template-areas branch from c5e72d6 to 4d47872 Compare May 15, 2023 18:24
Copy link
Member

@pixelflips pixelflips 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 recommend checking w/ Design on the missing title by default, but definitely not a show-stopper. Even if it's just to alert them 🤣 that we now support that use case.

LGTM! 👍🏼

@pixelflips pixelflips requested a review from a team May 15, 2023 18:42
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.

Looks good to me

@ju-Skinner ju-Skinner merged commit b67d972 into develop May 16, 2023
@ju-Skinner ju-Skinner deleted the fix/alert-grid-template-areas branch May 16, 2023 16:00
@ju-Skinner ju-Skinner mentioned this pull request May 16, 2023
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