-
Notifications
You must be signed in to change notification settings - Fork 8
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(cxl-ui): [cxl-notification] document renderer function #245
base: master
Are you sure you want to change the base?
Conversation
size-limit report 📦
|
@@ -5,6 +5,7 @@ export class CXLNotification extends Notification { | |||
return 'cxl-notification'; | |||
} | |||
|
|||
// Provide a default renderer which appends all child nodes to the notification root element. |
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.
Well I can see that from the code, but what's the need?
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.
Well I can see that from the code, but what's the need?
Maybe this clears it: https://lit.dev/docs/components/shadow-dom/#slots
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.
Updated to reflect the need for this strategy as well as a reference to the Slack discussion (https://cxlworld.slack.com/archives/C01JABH8AHX/p1662121897793169).
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.
@anoblet maybe add this too: https://lit.dev/docs/components/shadow-dom/#slots
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.
@saas786 I can add it you would like. Though we aren't using slots here. There's no slot
or slot name=""
node. All we are doing is moving direct children to vaadin-notification-card
.
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.
@saas786 I can add it you would like. Though we aren't using slots here. There's no
slot
orslot name=""
node. All we are doing is moving direct children tovaadin-notification-card
.
Not specifically because of slots
, but rather DOM rendering point of view, as far as I am aware, child elements won't render otherwise.
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.
Updated
845ee16
to
1f464a2
Compare
1f464a2
to
8e17f1c
Compare
No description provided.