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

Support parent dir for RTL timeline and fix a small bug for editable point items #2235

Merged
merged 7 commits into from
Oct 27, 2016

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
// Create the DOM, props, and emitter
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