Skip to content
This repository has been archived by the owner on Jan 19, 2023. It is now read-only.

Use cds-alert #2438

Merged

Conversation

xtreme-vikram-yadav
Copy link
Contributor

What this PR does / why we need it:

  • Uses cds-alert for alerts.
  • Refactors alert banners to use shared alert component

Which issue(s) this PR fixes

@xtreme-vikram-yadav xtreme-vikram-yadav changed the title Use cds-alert for alerts Use cds-alert May 10, 2021
@mklanjsek
Copy link
Contributor

This is really cool, but we need a way to document and showcase it, so adding one or two storybook stories would be helpful.

Copy link
Contributor

@mklanjsek mklanjsek 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! Before merging, can you just make one small change for card: cds-alert-group is too close to the form title, it needs something like margin-bottom: 10px;

Also a question: why is alert inside modal not consuming the whole parent width like alerts inside other components?

@GuessWhoSamFoo
Copy link
Contributor

The storybook alert should be under its own group rather than adding alerts to every relevant component in storybook. (e.g. show a summary and/or modal with alert under the Alert component)

@mklanjsek
Copy link
Contributor

The storybook alert should be under its own group rather than adding alerts to every relevant component in storybook. (e.g. show a summary and/or modal with alert under the Alert component)

@GuessWhoSamFoo Since alert component is not exported as official Octant component, I thought just adding the alerts to stories that use it in practice would be sufficient?

@GuessWhoSamFoo
Copy link
Contributor

Okay, I retract that statement. LGTM

Copy link
Contributor

@mklanjsek mklanjsek left a comment

Choose a reason for hiding this comment

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

Please fix the css and merge.

- Add button group to alerts

[vmware-archive#2303]

Signed-off-by: Vikram Yadav <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add standard alerts component
3 participants