search

PolymerElements / paper-dialog-behavior

Makes an element a Material Design dialog
18 stars 22 forks source link

buttons class colides with my light DOM, could it use slot to distribute the elements? #122

Closed tomalec closed 6 years ago

tomalec commented 6 years ago

Description

In my app, I use button class for styling to make everything pink and rain emojis when clicked.

paper-dialog element requires using the very same .button class in the light dom to distribute (?) and style elements, according to internal implementation details of it. Shouldn't it use slot attribute for that?

Expected outcome

I should be able to use this element without polluting my global styles scope, and restricting usage of class names.

Actual outcome

I cannot use the class button for my own needs.

Live Demo

https://jsbin.com/nigaka/edit?html,output

Browsers Affected

valdrinkoshi commented 6 years ago

The current implementation uses the .buttons class only for styling here https://github.com/PolymerElements/paper-dialog-behavior/blob/768729d22677442a833a2403df08173aa2a339dd/paper-dialog-shared-styles.html#L74 This style is applied only in the scope where paper-dialog is declared - if you have a paper-dialog in another element's shadow root, the style is applied only there.

We cannot change this as it would be a breaking change. Consider adding a style to undo these changes inside paper-dialogs - paper-dialog > .buttons { background-color: transparent } - or using another class

tomalec commented 6 years ago

Thanks for the reply.

We are trying to add <paper-dialog> to already existing, quite complex app, so changing entire system to use different class name just to use this element here is not an option.

Undoing whatever is done by all ... .buttons ...{} rules would be unstable and hard to maintain.

So far we are happy with something that seems to me as non-breaking change:

:host > ::slotted(.paper-dialog-buttons),
:host > ::slotted(.buttons) {

see https://github.com/PolymerElements/paper-dialog-behavior/pull/124/

I believe, prefixing it with an element name is quite safe approach. Having two custom elements with same name is a problem on its own, and the chance somebody is already using paper-dialog-buttons for other purposes is rather small.

valdrinkoshi commented 6 years ago

I'm hesitant on adding a new rule, as the same problem applies also for other selectors e.g. https://github.com/PolymerElements/paper-dialog-behavior/blob/768729d22677442a833a2403df08173aa2a339dd/paper-dialog-shared-styles.html#L46-L47

https://github.com/PolymerElements/paper-dialog-behavior/blob/768729d22677442a833a2403df08173aa2a339dd/paper-dialog-shared-styles.html#L58-L59

The suggested change would require to update the DOM with the new class, right? I'd like to explore some other options to see if they'd work for you:

/ or /

.buttons:not(.paper-dialog-buttons) { ... } /* ^ this would require to update also the DOM to something like

//...
... */ ``` - create a custom element that extends `paper-dialog-behavior` instead of using `paper-dialog` - notice that [`paper-dialog` implementation](https://github.com/PolymerElements/paper-dialog/blob/master/paper-dialog.html) is just implementing 2 behaviors + including the paper-dialog-styles in its shadowRoot. Here a sample implementation of ``: ```html
tomalec commented 6 years ago

I'm hesitant on adding a new rule, as the same problem applies also for other selectors e.g.

It does not, because those selector does not require me to use the class in the light DOM with colliding name.

The suggested change would require to update the DOM with the new class, right?

It does not, at all.

You can still use:

<paper-dialog>
    <h2>Header</h2>
    <div>Dialog body</div>
    <div class="buttons">
        <paper-button dialog-dismiss>Cancel</paper-button>
        <paper-button dialog-confirm>Accept</paper-button>
    </div>
</paper-dialog>
as well as 
<paper-dialog>
    <h2>Header</h2>
    <div>Dialog body</div>
    <div class="paper-dialog-buttons">
        <paper-button dialog-dismiss>Cancel</paper-button>
        <paper-button dialog-confirm>Accept</paper-button>
    </div>
</paper-dialog>

So there is no "breaking" in this change.

:not(paper-dialog) > .buttons { ... }

It increases specificity, which is a tough problem to track in big, modular system.

.buttons:not(.paper-dialog-buttons) { ... } ...

Again, it increases specificity. Also, if I already require <div class="buttons paper-dialog-buttons"> why do I need the buttons in first place?

create a custom element that extends paper-dialog-behavior instead of using paper-dialog

This means creating and maintaining my own fork of paper-dialog, that uses paper-dialog-behavior in exactly same way, just includes different stylesheet? That means almost forking two repos, as I'd have to track all changes made to paper-dialog-shared-styles to apply them back again.

I think requiring a non-prefixed, commonly used class breaks the Web Components story:

"Hey, we build modern modular, web app. Thanks to Custom Element and Shadow DOM, implementation details are now hidden behind just HTML Elements, all you see is pure content, clean DOM API with no leaky styles and div soup. BTW, just don't use .buttons class for your styling, as it break this tiny little dialog in some cases."

None of native elements requires class to be rendered correctly. Composition (<detail><summary>), or attribute (selected) is enough. What about using custom attribute, like

<paper-dialog>
    <h2>Header</h2>
    <div>Dialog body</div>
    <div paper-dialog-buttons>
        <paper-button dialog-dismiss>Cancel</paper-button>
        <paper-button dialog-confirm>Accept</paper-button>
    </div>
</paper-dialog>
warpech commented 6 years ago

@valdrinkoshi any comment on why paper-dialog-behavior uses a class name, not a slot?

For apps that want to make use of Shadow DOM with clean scope of CSS classes, I believe slotting buttons is the way to go.

Once you allow to slot the buttons, the .button class can be opt-out by using <paper-dialog no-classes>. So then your rule can be changed to:

:host:not([no-classes]) > ::slotted(.buttons)

WDYT?

valdrinkoshi commented 6 years ago

@tomalec I was trying to make the case that paper-dialog already styles all the direct children (h2, .buttons, .no-padding, *), and the user already needs to override those. Wouldn't adding a new class/attribute rule for .buttons justify adding a class also for the others too?

I do agree that .buttons is too generic and between all the options we discussed probably #124 is the safest approach 👍

@warpech it's mainly because of legacy code - the class name selector was there since the initial implementation in https://github.com/PolymerElements/paper-dialog-behavior/pull/1

https://github.com/PolymerElements/paper-dialog-behavior/blob/96ea4131524ae2236c599eea410e8e40685cb7e4/paper-dialog-common.css#L48

I assume it was added as a class so that elements implementing paper-dialog-behavior could use it to distribute it under ShadowDOM v0. @cdata @notwaldorf is this assumption correct?

We cannot use the slot selector because under ShadowDOM v1 <slot> will not distribute an element with slot="something". In ShadowDOM v0, <content> would distribute any element into the shadowRoot - e.g. see this in Chrome http://jsbin.com/paqulabude/3/edit?html,output So using a slot selector would be a breaking change as elements like paper-dialog would not distribute those elements under ShadowDOM v1

tomalec commented 6 years ago

@valdrinkoshi Thanks for merging :)

Wouldn't adding a new class/attribute rule for .buttons justify adding a class also for the others too?

I think styling h2, * children is not so intrusive as squatting a class name. Therefore I'd only add class for .no-padding (.paper-dialog-no-padding)

@warpech I think slot is not an option as it would not allow to cover such case:

    <paper-dialog id="dialog">
      <h2>Header</h2>
      <paper-dialog-scrollable>
        Lorem ipsum...
      </paper-dialog-scrollable>
      <div class="paper-dialog-buttons">
        <paper-button dialog-dismiss>Cancel</paper-button>
      </div>
      <h3>Sub section</h3>      
      <p>... dolor sit amet, ...</p>
      <div class="paper-dialog-buttons">
        <paper-button dialog-confirm>Accept</paper-button>
      </div>
    </paper-dialog>

as you cannot do:

<paper-dialog-shadowroot>
  <div id="dialog-frame">
     <slot name="heading">
     <slot></slot>
     <slot name="buttons"></slot><!-- <content select="[slot='buttons']"> -->
     then another part of default content <div><slot></slot></div>
     and another bunch of <slot name="buttons"></slot>
  </div>
</paper-dialog-shadowroot>

I have no idea whether anybody is using it that way, but as this is technically possible now, it would've been a breaking change.