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

Feature/enable viewfolder dragging #4

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

Conversation

KaminerD
Copy link

@KaminerD KaminerD commented May 8, 2018

related to OO-1352

Copy link

@guycastel guycastel left a comment

Choose a reason for hiding this comment

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

Although this code is very patchy it's probably going to last so I scattered some styling comments and I also suggest:

  1. add comments to explain the actual purpose of some blocks.
  2. add () in multi-conditional ifs and calculations.

}

if (curIndex != -1 && curIndex > 0)
return childrenArr[curIndex - 1].clientHeight;

Choose a reason for hiding this comment

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

Can be reduced to if (curIndex > 0):

if (curIndex > 0) {
...
}

}

if (curIndex != -1 && curIndex < childrenArr.length - 1)
return childrenArr[curIndex + 1].clientHeight;

Choose a reason for hiding this comment

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

if (...) {
...
}

@@ -97,9 +152,34 @@ class UITree extends Component {
dragStart = (id, dom, e) => {
if (this.props.disableDragging) {return;}
if ((this.props.disableRootDrag) && (id == 1)) {return;}
if (id == 2 && this.props.tree.module === "ROOT") {return;}

Choose a reason for hiding this comment

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

For above 3 ifs:

if (...) {
  return;
}



// exit from presets tree if its not favorites module
if (this.props.tree.module !== "ROOT" && this.props.tree.module !== "Favorites") {return;}

Choose a reason for hiding this comment

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

if (...) {
...
}

let nextElement = this.childrenRoot.children[0];

while (nextElement.innerText != this.dragging.dom.innerText &&
!nextElement.classList.contains("placeholder"))

Choose a reason for hiding this comment

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

while ((nextElement.innerText != this.dragging.dom.innerText) && 
       (!nextElement.classList.contains("placeholder"))) {
...
}

if (!this._start)
{
this.dragEnded(this.state.tree);
}

Choose a reason for hiding this comment

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

if (...) {
...
}

dragStarted = () =>
{
if (this.props.onDragStarted) this.props.onDragStarted();
};

Choose a reason for hiding this comment

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

xxx = () => {
  ... 2 space indented code ...
};

dragEnded = (tree) =>
{
if (this.props.onDragEnded) this.props.onDragEnded(tree.obj);
};

Choose a reason for hiding this comment

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

xxx = () => {
  ... 2 space indented code ...
};

if (placeholder)
{
curIndex = childrenArr.indexOf(placeholder);
}

Choose a reason for hiding this comment

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

if (...) {
...
}

getBaseItemHeight = () => {
let childrenArr = Array.from(this.childrenRoot.children);
let heightArray = childrenArr.filter(x => x.clientHeight && x.clientHeight > 0).map(x => x.clientHeight);
let baseHeight = Math.min.apply(null, heightArray);

Choose a reason for hiding this comment

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

I think that if this is ES6 you can Math.min(...heightArray)

@guycastel guycastel assigned guycastel and noammat and unassigned shimonmagal and guycastel Feb 3, 2019
@noammat noammat closed this Nov 24, 2019
@noammat noammat deleted the feature/enable-viewfolder-dragging branch November 24, 2019 17:02
@noammat noammat restored the feature/enable-viewfolder-dragging branch November 24, 2019 17:16
@noammat noammat reopened this Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants