search

PolymerElements / paper-drawer-panel

A Material Design two-section responsive panel
https://www.webcomponents.org/element/PolymerElements/paper-drawer-panel
25 stars 42 forks source link

Make paper-drawer-panel#rightDrawer default to true in RTL #130

Closed danbeam closed 8 years ago

danbeam commented 8 years ago

possibly fixes https://github.com/PolymerElements/paper-drawer-panel/issues/101

i also thought about creating a

directionChanged: function() {
  this.rightDrawer = this._isRTL();
},

for folks to call if directionality changes, but:

/cc @blasten @frankiefu

blasten commented 8 years ago

Thanks for fixing this @danbeam. The test is still failing, do you know why?

danbeam commented 8 years ago

according to @esprehn I'm hitting a bug where calling getComputedStyle() outside of the document or in a <template> returns an empty string rather than (in direction's case) 'ltr' or 'rtl'.

notwaldorf commented 8 years ago

@danbeam does it help if you set rightDrawer in attached() instead rather than in the properties block (i.e. to make sure that you're in the document when it matters)

blasten commented 8 years ago

would this help?

var drawer;

setup(function() {
  drawer = fixture('rtl-drawer');
});

test('rtl-drawer defaults rightDrawer to true', function(done) {
   flush(function() {
     expect(drawer.rightDrawer).to.be.true;
     done();
    });
});
keanulee commented 8 years ago

One of the reasons why tests are failing is because you are overriding rightDrawer based on RTL even if it has been explicitly set. One alternative is to have rightDrawer default as null in the properties object, then initialize it in attached based on RTL only if it is null (and not false). Not sure if this is good conventionally, but it works. WDYT @danbeam @notwaldorf @blasten ?

rightDrawer: {
  type: Boolean,
  value: null
}

...

attached: function() {
  if (this.rightDrawer === null) {
    this.rightDrawer = this._isRTL();
  }
}
danbeam commented 8 years ago

@keanulee that helps and fixes the issue of this:

var paperDrawerPanel = document.createElement('paper-drawer-panel');
paperDrawerPanel.rightDrawer = ...;
document.body.appendChild(paperDrawerPanel);  // overriden

but doesn't pass all tests yet

keanulee commented 8 years ago

@danbeam There's only one failing test, and here's the fix :) In test/small-devices.html, change the contents of the style tag to be the following instead:

<style is="custom-style">
  body {
    margin: 0;
    padding: 0;
  }
  .notransition {
    --paper-drawer-panel-drawer-container: {
      transition: none !important;
    };
  }
</style>

The tests actually passes in ShadyDOM (i.e. improper style scoping), but breaks in ShadowDOM because .notransition * doesn't pierce the shadow root. Applying this mixin will, and tests should pass.

blasten commented 8 years ago

@danbeam friendly ping.

danbeam commented 8 years ago

so, what I don't like about my patch: rightDrawer is now undefined until attached().

@blasten or @keanulee: thoughts?

additionally, if rightDrawer changes in attached(), does the newly create element animatedly switch to the right or does it show correctly instantly? I assume instantly because of the way that templates are asynchronously rendered, but wasn't sure.

keanulee commented 8 years ago

@danbeam Yeah, that's not ideal, and it does cause the animation. In <app-drawer> (the next iteration of <paper-drawer-panel>, we're introducing an align property that can be set to start/end, which are RTL compatible, or explicitly to left/right. That would be a breaking API change, so it's hard to add it here.

One suggestion is to try to set rightDrawer outside of <paper-drawer-panel> based on document.dir. It's unfortunate that this element doesn't support this by default, but this is probably the cleanest solution for now. http://jsbin.com/lameliqovi/edit?html,output

danbeam commented 8 years ago

Chrome already does set it externally.

It turns out Chrome's UI sets <html dir=rtl> after elements are attached (too late in the process).

That doesn't mean <paper-drawer-panel> shouldn't have a better, dynamic default, but the current implementation (and the fact that we must wait until attached() to accurately determine _isRTL) is kind of a bummer.

I did implement a prototype along the lines of:

Polymer({
  properties: {
    _rightDrawer: Boolean,
    _defaultDirection: String,

    get rightDrawer() {
      return this._rightDrawer === undefined ? this._defaultDirection == 'rtl' : this._rightDrawer;
    },
    set rightDrawer(rightDrawer) {
      this._rightDrawer = rightDrawer;
    },
  },

  attached: function() {
    this._defaultDirection = window.getComputedStyle(this).direction;
  },
});

but didn't find it much better and didn't know how property change accessors would work (maybe switch them all from (..., rightDrawer, ...) to (..., _rightDrawer, _defaultDirection, ...) or call them programmatically?

at least this way we could determine which thing changed and only animatedly change if _rightDrawer was changed?

blasten commented 8 years ago

The conclusion here was that the direction should be set externally.

blasten commented 8 years ago

@danbeam should we close this PR?