-
Notifications
You must be signed in to change notification settings - Fork 78
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
feat(panel): add content-bottom slot #9311
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.
Aside from a couple of comments, this LGTM! 🎉
@@ -328,6 +331,10 @@ export class Panel | |||
this.hasFab = slotChangeHasAssignedElement(event); | |||
}; | |||
|
|||
private contentBottomSlotChangeHandler = (event: Event): void => { | |||
this.hasContentBottom = slotChangeHasAssignedElement(event); |
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 may have missed this in the content-top
slot PR, but should this use slotChangeHasContent
to consider both text and elements?
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.
Everywhere I look we use slotChangeHasAssignedElement
, and slotChangeHasContent
is only used once, in
scrim
. Do we want to handle both (elements and text) in all cases, or is checking for text not needed everywhere?
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.
@Elijbet we only need to consider slotChangeHasContent
for unnamed default slots. Otherwise, an element is used with a slot="x"
.
Related Issue: #8979
Summary
Add a new
content-bottom
slot to thepanel
component.