Skip to content
This repository was archived by the owner on Jul 29, 2019. It is now read-only.

Conversation

@yotamberk
Copy link
Member

We can now define a rtl timeline buy reading the closest parent dir. closes #2027.

and fix a small bug I caused in a previous PR which made editable point items unuseable...

Copy link
Member

@mojoaxel mojoaxel left a comment

Choose a reason for hiding this comment

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

Nice!
But I think we should also check it the <html> tag has a direction. But this could be done in a separate PR if you want.

@yotamberk
Copy link
Member Author

@mojoaxel it already does it =)

@yotamberk yotamberk merged commit b96566e into visjs:develop Oct 27, 2016
this._create(container);

if (!options || (options && typeof options.rtl == "undefined")) {
this.options.rtl = window.getComputedStyle(this.dom.root, null).direction == "rtl";
Copy link
Member

@mojoaxel mojoaxel Oct 27, 2016

Choose a reason for hiding this comment

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

This line should be better something like this:

// check if the timelines root element or one of its parents has a direction
var directionFromDom, domNode = this.dom.root;
while (!directionFromDom && domNode) {
    directionFromDom = window.getComputedStyle(domNode, null).direction;
    domNode = domNode.parentElement;
}
this.options.rtl = (directionFromDom && (directionFromDom.toLowerCase() == "rtl"));

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add this to another PR? Or do you want to revert?

Copy link
Member

Choose a reason for hiding this comment

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

You could just change it and push the changes to your branch. Would be cool, if you test it before, I didn't ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested it and submitted another PR =)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants