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.

Sign up for GitHub

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

Jump to bottom

Issue 414 #425

Merged
merged 5 commits into from
May 17, 2017
Merged

Issue 414 #425

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8507dce
fix on flow issues
piotr-gawron Jun 27, 2016
240b70b
lint issues
piotr-gawron Jun 27, 2016
6f78d3f
lint issues
piotr-gawron Jun 27, 2016
04d0da5
fix that makes unit tests to pass
piotr-gawron Jul 18, 2016
8480090
small bug fixed: when div where pileup is visualized was inside anoth…
piotr-gawron Jul 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/main/Root.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,19 @@ class Root extends React.Component {
contigList: string[];
range: ?GenomeRange;
settingsMenuKey: ?string;
updateSize: boolean;
};
trackReactElements: Array<Object>; //it's an array of reactelement that are created for tracks

constructor(props: Object) {
super(props);
this.state = {
contigList: this.props.referenceSource.contigList(),
range: null,
updateSize: false,
settingsMenuKey: null
};
this.trackReactElements = [];
}

componentDidMount() {
Expand Down Expand Up @@ -81,12 +85,16 @@ class Root extends React.Component {
}

makeDivForTrack(key: string, track: VisualizedTrack): React.Element {
//this should be improved, but I have no idea how (when trying to
//access this.trackReactElements wih string key, flow complains)
var intKey = parseInt(key);
var trackEl = (
<VisualizationWrapper visualization={track.visualization}
range={this.state.range}
onRangeChange={this.handleRangeChange.bind(this)}
source={track.source}
referenceSource={this.props.referenceSource}
ref = {(c) => {this.trackReactElements[intKey]=c}}
/>);

var trackName = track.track.name || '(track name)';
Expand Down Expand Up @@ -158,6 +166,16 @@ class Root extends React.Component {
</div>
);
}

componentDidUpdate(prevProps: Props, prevState: Object) {
if (this.state.updateSize) {
for (var i=0;i<this.props.tracks.length;i++) {
this.trackReactElements[i].setState({updateSize:this.state.updateSize});
}
this.state.updateSize=false;
}
}

}
Root.displayName = 'Root';

Expand Down
10 changes: 9 additions & 1 deletion src/main/VisualizationWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ type Props = {

class VisualizationWrapper extends React.Component {
props: Props;
state: {width: number; height: number};
state: {width: number; height: number; updateSize: boolean};
hasDragBeenInitialized: boolean;
onResizeListener: Object; //listener that handles window.onresize event

constructor(props: Object) {
super(props);
this.hasDragBeenInitialized = false;
this.state = {
updateSize: false,
width: 0,
height: 0
};
Expand All @@ -47,6 +48,7 @@ class VisualizationWrapper extends React.Component {
updateSize(): any {
var parentDiv = ReactDOM.findDOMNode(this).parentNode;
this.setState({
updateSize: false,
width: parentDiv.offsetWidth,
height: parentDiv.offsetHeight
});
Expand Down Expand Up @@ -122,6 +124,12 @@ class VisualizationWrapper extends React.Component {
}
}

componentWillUpdate(nextProps:Props, nextState: Object) {
if (nextState.updateSize) {
this.updateSize();
}
}

render(): any {
const range = this.props.range;
const component = this.props.visualization.component;
Expand Down
36 changes: 36 additions & 0 deletions src/main/pileup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,37 @@ function create(elOrId: string|Element, params: PileupParams): Pileup {
ReactDOM.render(<Root referenceSource={referenceTrack.source}
tracks={vizTracks}
initialRange={params.range} />, el);

//if the element doesn't belong to document DOM observe DOM to detect
Copy link
Member

Choose a reason for hiding this comment

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

How about using some underscore utilities to make this code more concise? For example:

  //if the element doesn't belong to document DOM observe DOM to detect
  //when it's attached
  var observer = null;
  if (!document.body.contains(el) && reactElement) {
    observer = new MutationObserver(mutations => {
      var updateSize = !_.chain(mutations)
        .filter(m => m.type === 'childList' && _.contains(m.addedNodes, el))
        .isEmpty()
        .value();
      reactElement.setState({updateSize});
    });

    // start observing document
    observer.observe(document, {childList: true});
  }

/* ... */

      // disconnect observer if it was created
      if (observer) {
        observer.disconnect();
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can do that, but for me this code is really hard to read and understand. This later on will influence time spent on debuging and improving code.

But this is my opinion. Maybe for Javascript developers this syntax is obvious and easy to read :).

Copy link
Member

Choose a reason for hiding this comment

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

your version is definitely more explicit, but let's delegate some of the logic to underscore for clarity. The for-loop-based check is a pretty common pattern, so I think it is safe to replace that with the filter method. What we can do is to add a few sentences in between the chain so that it becomes more obvious for future developers taking a look at the code.

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 will leave it to you. I don't know underscore notation.

//when it's attached
var observer = null;
if (!document.contains(el)) {
Copy link
Member

Choose a reason for hiding this comment

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

TIL that document.contains is not a standard thing: http://stackoverflow.com/questions/20557115/mocha-casperjs-headless-testing-document-contains-undefined

but if (!document.body.contains(el)) { works fine as suggested in that SO answer. This should fix the issue with the headless tests.

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 dissagree with this answer at SO. Here you have better source:
https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

It is a standard, the only remark is that IE doesn't support it for document but only for document.body.

I can change it to document.body if it solves the problem.

Anyway, I took it from this answer at SO:
http://stackoverflow.com/questions/5629684/how-to-check-if-element-exists-in-the-visible-dom/5629730#5629730

Copy link
Member

Choose a reason for hiding this comment

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

as an alternative, you can do:

var contains = document.contains || document.body.contains;
if (!contains(el)) {
...
}

PhantomJS framework can sometimes surface issues like this; but, the pros still outweigh its cons, so we are still sticking to it. Also, any such issue will affect people trying to render views in headless mode, so it would be great if we address this even though it is a weird workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this doesn't work: I get some strange Invalid invocation error in Chrome, Firefox reports different error...

But I fixed it by replacing: document.contains with document.body.contains

observer = new MutationObserver(function(mutations) {
mutations.forEach(function(mutation) {
if (mutation.type === 'childList') {
var added= false;
for (var i=0; i<mutation.addedNodes.length;i++) {
if (mutation.addedNodes[i]===el) {
added = true;
}
}
if (added) {
if (reactElement) {
reactElement.setState({updateSize:true});
} else {
throw 'ReactElement was not initialized properly';
Copy link
Member

Choose a reason for hiding this comment

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

can you give me an example why we might need this error? Looks like this type of an error is out of the scope for pileup.js?

Copy link
Contributor Author

@piotr-gawron piotr-gawron Jul 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Well, I don't like when the code fails to do what it's supposed to do and don't give any hint what went wrong.
It helps debugging, testing etc.

But it's your code so I can remove it.

Copy link
Member

@armish armish Jul 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I say we keep it if you hit this problem when trying to solve this issue. The check for the reactElement happens deep within that if block, though. Maybe it makes sense to move it to the top?

also: it is also your code now; so we get to decide on the best practice together ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can move it up. But it must be inside MutationObserver constructor.
There might be some issues due to lifecycle of react component (I'm not familiar with it at all):

  • consider object creation
  • object destruction - which method will be called when - maybe observer still will be running when react element is gone??

Anyway, where do you want to move it? Inside forEach loop? (line 90)

}
}
}
});
});
// configuration of the observer:
var config = { attributes: true, childList: true, characterData: true, subtree: true };
Copy link
Member

Choose a reason for hiding this comment

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

if we are only looking for mutations of type childList, do we need all the other ones? I guess subtree is relevant, but if not let's drop the others and only keep the childList.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably you are right, I will check it.

Copy link
Member

Choose a reason for hiding this comment

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

thanks a lot!


// start observing document
observer.observe(document, config);
Copy link
Member

Choose a reason for hiding this comment

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

I have no experience with the MutationObserver API, but do you think observing the whole document DOM tree will result in some performance issues for us? Do you have an idea about how we can only watch some relevant elements and not the whole document?

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 also don't have any experience. I found an article here:
https://hacks.mozilla.org/2012/05/dom-mutationobserver-reacting-to-dom-changes-without-killing-browser-performance/
They mention it as the proper way to do it when performance is important (I know it's 4 years old, but couldn't find better way to do that).

I was thinking about watching proper elements, but I couldn't find any better solution. My first idea was quite opposit - watch our node element if it's parent has changed, but I couldn't make it work (even though that MutationObserver in theoury allows to observe modification of node attributes - maybe parent is not treated as an attribute...).
This is why I added this observer only when it's not in DOM. Maybe we should disconnect observer when element is attached to DOM.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. If you don't have any performance issues in your complex application after this change, it is unlikely that we will have that for other simpler applications. Thanks for the pointer!

}

return {
setRange(range: GenomeRange) {
if (reactElement === null) {
Expand All @@ -104,6 +135,11 @@ function create(elOrId: string|Element, params: PileupParams): Pileup {
reactElement = null;
referenceTrack = null;
vizTracks = null;

// disconnect observer if it was created
if (observer !== null && observer !== undefined) {
observer.disconnect();
}
}
};
}
Expand Down