Skip to content
This repository has been archived by the owner on Dec 19, 2024. It is now read-only.

Add global-only RTL mirroring. #74

Merged
merged 1 commit into from
Aug 30, 2017
Merged

Add global-only RTL mirroring. #74

merged 1 commit into from
Aug 30, 2017

Conversation

cdata
Copy link
Contributor

@cdata cdata commented Aug 29, 2017

RTL detection based on CSS styles in subtrees has timing issues. In some circumstances, the value of the direction style property will be empty string while an element is booting up even though it should probably read as "rtl".

This change proposes a fix where an addition configuration (use-global-rtl-attribute) of the iconset tells it to use the value of the dir attribute on either <body> or <html>, whichever is available.

Fixes #64

var globalElement =
(document.body && document.body.hasAttribute('dir'))
? document.body
: document.documentElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

is documentElement ever null?

Copy link
Contributor

Choose a reason for hiding this comment

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

is it html? the og comment promised we also look at html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

document.documentElement is the <HTML> node and should never be null in any reasonable circumstance AFAIK.

* html elements (measured on document.body or document.documentElement as
* available).
*/
useGlobalRtlAttribute: {
Copy link
Contributor

Choose a reason for hiding this comment

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

uhhh maybe RTL to match __targetIsRT? 🚲🏡

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you spell that as an attribute for me? :)


suite('and dir="rtl" on <html>', function() {
setup(function() {
document.documentElement.setAttribute('dir', 'rtl');
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe call out html explicitly? is there a document.html or am i smoking crack?

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 definitely crack

@cdata cdata merged commit 254fc46 into master Aug 30, 2017
@cdata cdata deleted the global-only-mirroring branch August 30, 2017 18:45
@bicknellr
Copy link
Contributor

Is this considered solving #57?

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

Successfully merging this pull request may close these issues.

If icons are created too early, they are not mirrored in RTL correctly.
6 participants