-
Notifications
You must be signed in to change notification settings - Fork 20
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
[Feature] go-toast maxHeight & icon overrides #819
Conversation
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.
This looks good Graham, just put some nitpicky comments that may or may not be good ideas
<ng-template #messageContentElse> | ||
<p | ||
class="go-toast-content__message" | ||
[innerHTML]="message"> |
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.
I feel that the naming here may be a little confusing just because message is a string but messageContent is a template ref. I feel like messageContentTemplate (or something like that) would be a little more explicit just from the standpoint of reading the html.
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.
This is a fair point and a good suggestion. In this instance, none of this is actually new code. I am just restructuring the HTML to accommodate the CSS changes. Ideally, I'd like to refrain from making unrelated code changes so I think I'll leave it as is here.
display: flex; | ||
flex: 1; | ||
justify-content: flex-end; | ||
padding: .875rem 1.25rem; | ||
padding: 1.25rem; |
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.
Would it be a good idea to make variables for some of these numeric values?
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.
Sure. I think that's a fair point here. I did this with the padding because we use the same 1.25rem
value in multiple spots.
<h4 class="go-heading-4">icon</h4> | ||
<p class="go-body-copy go-body-copy--no-margin"> | ||
Changes the icon that is displayed within the toast. This is set automatically for each type, but if a value | ||
is passed for this binding it will override all of those type icons. See the |
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.
So I see this is for the docs so I'm not sure it really matters, would it be a good idea to store text like this in a variable somewhere and use {{ interpolation }}?
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.
In this case, because it's just documentation and the value will never change I think it's fine to just have plain text here. Using interpolation on text that doesn't change is an extra thing that Angular will need to keep track of that it doesn't need to. Good thinking though!
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, approving!
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Toasts do not have a max height.
Toast icons are set programmatically based on
type
binding.Resolves #815
What is the new behavior?
Toasts now have a max height, with a new binding to provide the ability to disable the max height.
Toasts now have an
icon
binding that, when passed, will override all of the programmatically set icons for a toast.Does this PR introduce a breaking change?