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

danbeam · 2016-02-17T01:17:54Z

possibly fixes #101

i also thought about creating a

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

for folks to call if directionality changes, but:

I think it might be odd for some as a side effect
rightDrawer is already @public so this doesn't add much value (authors can do this themselves if they wish)

/cc @blasten @frankiefu

blasten · 2016-02-19T02:17:06Z

paper-drawer-panel.html

@@ -550,6 +552,10 @@
          this._transition = true;
        },

+        _isRTL: function() {
+          return window.getComputedStyle(this).direction == 'rtl';


blasten · 2016-02-19T02:19:52Z

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

danbeam · 2016-02-19T06:27:53Z

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 · 2016-02-19T06:33:21Z

@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 · 2016-02-19T19:17:14Z

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 · 2016-02-19T23:18:20Z

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 · 2016-02-19T23:34:50Z

@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 · 2016-02-20T02:27:47Z

@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 · 2016-02-24T18:39:52Z

@danbeam friendly ping.

danbeam · 2016-02-24T22:26:59Z

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

this could break those doing deep comparisons (i.e. rightDrawer === true) before attached()
rightDrawer being initially undefined might delay template rendering (it's used from various bindings) or cause more re-calculations when it changes (maybe?)

@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 · 2016-02-24T22:48:10Z

@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 · 2016-02-25T00:26:19Z

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 · 2016-03-21T17:09:23Z

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

blasten · 2016-03-22T18:01:45Z

@danbeam should we close this PR?

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

d2edb29

googlebot added the cla: yes label Feb 17, 2016

danbeam and others added 3 commits February 16, 2016 17:18

doh

287c331

fix tests?

8383dde

move to test/positioning.html and scope

2af0381

blasten added the p0 - critical label Feb 19, 2016

blasten reviewed Feb 19, 2016
View reviewed changes

danbeam added 2 commits February 18, 2016 23:17

move to attached()

4f75138

===

ac44641

remove Polymer.Base.async() call around RTL case

ce94713

allow setting rightDrawer before attached() to document

02c8afb

notransition fix

f7366cc

danbeam closed this Mar 22, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danbeam commented Feb 17, 2016

blasten Feb 19, 2016

blasten commented Feb 19, 2016

danbeam commented Feb 19, 2016

notwaldorf commented Feb 19, 2016

blasten commented Feb 19, 2016

keanulee commented Feb 19, 2016

danbeam commented Feb 19, 2016

keanulee commented Feb 20, 2016

blasten commented Feb 24, 2016

danbeam commented Feb 24, 2016

keanulee commented Feb 24, 2016

danbeam commented Feb 25, 2016

blasten commented Mar 21, 2016

blasten commented Mar 22, 2016

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

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

Conversation

danbeam commented Feb 17, 2016

blasten Feb 19, 2016

Choose a reason for hiding this comment

blasten commented Feb 19, 2016

danbeam commented Feb 19, 2016

notwaldorf commented Feb 19, 2016

blasten commented Feb 19, 2016

keanulee commented Feb 19, 2016

danbeam commented Feb 19, 2016

keanulee commented Feb 20, 2016

blasten commented Feb 24, 2016

danbeam commented Feb 24, 2016

keanulee commented Feb 24, 2016

danbeam commented Feb 25, 2016

blasten commented Mar 21, 2016

blasten commented Mar 22, 2016