-
Notifications
You must be signed in to change notification settings - Fork 307
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
Add an AlertModalComponent #100
Add an AlertModalComponent #100
Conversation
Also fixes table not centring properly in its parent container.
… border." This reverts commit f808411.
….show()`. Added new storybook sections. Updated Modal styles to match Carbon's. Fixed docs.
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.
Nice 🎉 just a couple small thoughts
Renamed `elRef` to `elementRef`.
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.
Couple of small stylistic things in the template, but otherwise, approved!
Also it seems like travis is sad for some reason, looks like lint errors |
Travis checks are now passing :) |
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.
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.
Nice implementation :)
src/modal/modal.stories.ts
Outdated
.add("Passive", () => ({ | ||
template: ` | ||
<app-alert-modal-story [modalType]="modalType" [headerLabel]="headerLabel" [title]="title" [text]="text" | ||
></app-alert-modal-story> |
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.
We don't normally put lone >
s on a separate line.
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.
There's some code added to library since you made the PR that could simplify a part of it if it's possible to use it here.
@@ -13,7 +13,7 @@ import { Modal, ModalService } from "../"; | |||
selector: "app-sample-modal", | |||
template: ` | |||
<ibm-modal> | |||
<ibm-modal-header (closeSelect)="closeModal()">Header text</ibm-modal-header> | |||
<ibm-modal-header (closeSelect)="closeModal()">header label</ibm-modal-header> |
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.
Yeah, label is fine, I was just trying to point out the lowercase to uppercase H
:)
Sorry for being a bit ambiguous.
…ular into AlertModalComponent
@@ -13,7 +13,7 @@ import { Modal, ModalService } from "../"; | |||
selector: "app-sample-modal", | |||
template: ` | |||
<ibm-modal> | |||
<ibm-modal-header (closeSelect)="closeModal()">Header text</ibm-modal-header> | |||
<ibm-modal-header (closeSelect)="closeModal()">header label</ibm-modal-header> |
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.
Can we get a capital H in header label
so it would be Header label
, pretty please? :)
You can even name it Modal heading
to match vanilla and React flavors.
Sure, would we be able to deliver it after and do incremental updates if
needed? I need to integrate that dialog in the project already :-)
Thanks,
Stan.
…On Thu, Sep 27, 2018, 4:07 PM Zvonimir Fras, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/modal/modal.stories.ts
<#100 (comment)>
:
> @@ -13,7 +13,7 @@ import { Modal, ModalService } from "../";
selector: "app-sample-modal",
template: `
<ibm-modal>
- <ibm-modal-header (closeSelect)="closeModal()">Header text</ibm-modal-header>
+ <ibm-modal-header (closeSelect)="closeModal()">header label</ibm-modal-header>
Can we get a capital H in header label so it would be Header label,
pretty please? :)
You can even name it Modal heading to match vanilla and React flavors.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#100 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABMgXYcCFQmZ67H-35BOb8wOgglvQVdKks5ufTAXgaJpZM4WynTs>
.
|
Sure that sounds fine with me |
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.
Let's get this merged 💯
🎉 This PR is included in version 1.8.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #95