-
Notifications
You must be signed in to change notification settings - Fork 486
Use cds-alert #2438
Use cds-alert #2438
Conversation
40ffb3c
to
2602c62
Compare
2ee30b4
to
2b58b7f
Compare
web/src/app/modules/sugarloaf/components/smart/notifier/notifier.component.html
Show resolved
Hide resolved
97d521b
to
58d72e2
Compare
61a9265
to
373efde
Compare
This is really cool, but we need a way to document and showcase it, so adding one or two storybook stories would be helpful. |
6ec2209
to
2003122
Compare
There was a problem hiding this 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?
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? |
Okay, I retract that statement. LGTM |
There was a problem hiding this 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.
2003122
to
c8e3495
Compare
- Add button group to alerts [vmware-archive#2303] Signed-off-by: Vikram Yadav <[email protected]>
c8e3495
to
d2e0c3b
Compare
What this PR does / why we need it:
Which issue(s) this PR fixes