-
Notifications
You must be signed in to change notification settings - Fork 331
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
can add CSS classes to paper dialog container #571
can add CSS classes to paper dialog container #571
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.
I like this idea. Each modal can have its customisations.
See my comments, please.
@@ -5,7 +5,7 @@ | |||
fixed=(unless parent true) | |||
class="md-dialog-backdrop" | |||
onClick=(action "outsideClicked")}} | |||
{{#paper-dialog-container outsideClicked=(action "outsideClicked")}} | |||
{{#paper-dialog-container class=(readonly dialogContainerClasses) outsideClicked=(action "outsideClicked")}} |
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 think that class
would be a better API than dialogContainerClasses
, no?
Since paper-dialog
is a tagless component, we can just transport the passed in classes to paper-dialog-container
.
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 named it dialogContainerClasses
for two reasons:
- consistency with other components (ex:
paper-select
:class=(readonly concatenatedTriggerClasses)
) - avoid collision because there is multiple component we can pass class to (container, inner, ..)
Or class
for the container
and innerClasses
for paper-dialog-inner
?
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.
IMO:
class
forpaper-dialog-inner
, since this is the dialog itselfdialogContainerClass
forpaper-dialog-container
paper-select component renders two things, a trigger and a dropdown. That's why we need to be more specific.
In this case this is just a "proxy" to render a single component, so there shouldn't be any confusion.
test('can specify dialog container classes', function(assert) { | ||
this.render(hbs` | ||
<div id="paper-wormhole"></div> | ||
{{paper-dialog dialogContainerClasses='flex-50 my-dialog-container'}} |
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.
For consistency project-wide, please use double quotes in everything template related.
6d8d581
to
523e560
Compare
17512de
to
14b8141
Compare
a337081
to
245046f
Compare
Perfect PR. 💯 |
Add the capability for the user to specify CSS classes to the paper dialog container if necessary.