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

Fix pan issue #657 #2617

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Fix pan issue #657 #2617

wants to merge 14 commits into from

Conversation

mosheDa
Copy link
Contributor

@mosheDa mosheDa commented Jan 23, 2017

This pull request fixes #657.

@Tooa Tooa added the Timeline label Jan 24, 2017
@@ -83,6 +83,9 @@ function Timeline (container, items, groups, options) {
this.components = [];

this.body = {
//moshe's code
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment

// attach to the DOM


// moshe's code
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove comment

var newEnd = end + distance;
}
else
{
Copy link
Contributor

Choose a reason for hiding this comment

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

brackets don't need a new line. should be } else {


if ((this.options.rtl && this.mouseLocation.mouseOnLeft)||
(!this.options.rtl && this.mouseLocation.mouseOnRight))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for new line

if(props.item.data.end) itemWidth = props.item.data.end.valueOf() - props.item.data.start.valueOf();
start = windowEnd - itemWidth;
} else
{
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for new line

}

this.body.dom.leftZone.onmouseleave = this.body.dom.leftContainer.onmouseleave = function()
{
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for new line

}

this.body.dom.rightZone.onmouseenter = this.body.dom.rightContainer.onmouseenter = function()
{
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for new line

}

this.body.dom.rightZone.onmouseleave = this.body.dom.rightContainer.onmouseleave = function()
{
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for new line

this.body.dom.rightZone.onmouseleave = this.body.dom.rightContainer.onmouseleave = function()
{
context.setMouseLeave(true);
// context.setMouseOnRight(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

// context.setMouseOnRight(false);
}


Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment and empty lines

@yotamberk yotamberk added this to the Minor Release v4.19 milestone Jan 24, 2017
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.

Please make the changes requested by @yotamberk Thanks!

@@ -83,6 +83,8 @@ function Timeline (container, items, groups, options) {
this.components = [];

this.body = {
getWindow: this.getWindow,
//
Copy link
Member

Choose a reason for hiding this comment

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

please remove this line.

@@ -51,6 +51,8 @@ Core.prototype._create = function (container) {
this.dom.shadowTopRight = document.createElement('div');
this.dom.shadowBottomRight = document.createElement('div');
this.dom.rollingModeBtn = document.createElement('div');
this.dom.leftZone = document.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a problem with these names. They are too generic. I think they should be renamed to: panningZoneLeft and panningZoneRight

this.dragEvent = null;

this.mouseLocation = {mouseOnLeft: false, mouseOnRight: false, mouseIsLeaving: false}
var context = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for this.

var context = this;

this.setMouseOnLeft = function(isOn){
context.mouseLocation.mouseOnLeft = isOn;
Copy link
Contributor

Choose a reason for hiding this comment

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

use this instead of context


this.dragEvent = null;

this.mouseLocation = {mouseOnLeft: false, mouseOnRight: false, mouseIsLeaving: false}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should not add another property referring to mouse properties. There is a this.touchParams which contains all the mouse position props. I'd attach these properties to this.touchParams instead of defining this.mouseLocation

@yotamberk
Copy link
Contributor

The changes look fine but the tests are not passing =(

@@ -83,6 +83,7 @@ function Timeline (container, items, groups, options) {
this.components = [];

this.body = {
getWindow: this.getWindow,
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i should add this function to this.body because i want to get the range in itemSet module and this.body sent to itemSet constructor


}

var context = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove context and use this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the context is needed to use setMouseOnLeft function in the mouse events for example: this.body.dom.panningZoneLeft.onmouseenter = function(){
context.setMouseOnLeft(true);
}

@mojoaxel mojoaxel modified the milestones: Minor Release v4.20, Patch Release v4.20.1 May 22, 2017
@mojoaxel mojoaxel removed this from the Minor Release v4.20 milestone May 22, 2017
@wimrijnders
Copy link
Contributor

@mosheDa The check is failing, could you please look at it? Also, there are still outstanding review items to process. A resolution of these would be greatly appreciated.

@mosheDa
Copy link
Contributor Author

mosheDa commented Jul 9, 2017 via email

@shawn-mcginty
Copy link

Is this coming down the pike anytime soon? Would love to have this 👍

@yotamberk
Copy link
Contributor

@mosheDa still waiting 😉

.panningZoneLeft{
left: 0;

Copy link
Member

Choose a reason for hiding this comment

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

Please remove empty line

@mbroad
Copy link

mbroad commented Sep 5, 2017

@mosheDa - are you looking for someone to carry these changes and finish this out for you? Or are you planning on continuing this work?

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

Successfully merging this pull request may close these issues.

7 participants