Skip to content
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

Use effective direction when attached to mirror slider X #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danbeam
Copy link
Contributor

@danbeam danbeam commented May 17, 2017

Fixes #190

@danbeam danbeam requested a review from keanulee May 17, 2017 00:19
@@ -483,6 +488,10 @@
'up pageup end': '_incrementKey'
},

attached: function() {
this._isRTL = window.getComputedStyle(this)['direction'] === 'rtl';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getComputedStyle() in attached is costly, and could degrade performance significantly especially if there are multiple instances of this element. Don't know of any alternatives tho - thoughts @frankiefu?

Copy link
Contributor

@frankiefu frankiefu May 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, as @keanulee points out getComputedStyle will force layout and so not really ideal here. There is :dir() CSS pseudo-class which will solve this problem but it's not implemented in Chrome yet. So without :dir or host-context one workaround is to manually check for dir attribute on the parent and the body. @sorvell did a quick prototype on this: https://glitch.com/edit/#!/fern-scarf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frankiefu yep, I know about :dir() and how it doesn't work in Chrome yet.

Did you read the bug? #190 it talks about how just checking [dir] is not good enough.

Copy link
Contributor

@frankiefu frankiefu May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately without better platform support this is the limitation we have to endure without sacrificing performance. It seems to me the biggest issue is currently there is no way to force the direction of the slider to be LTR when the page's has a dir="rtl". I'd suggest we just make it to support if someone wants to override the dir it has to be set on paper-slider <paper-slider dir="ltr">.

@keanulee
Copy link
Contributor

keanulee commented Jun 27, 2017

See #199, which covers some of this functionality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants