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

ENCD-5537 save browser position on sort #3555

Merged
merged 1 commit into from
Oct 2, 2020

Conversation

emro
Copy link
Contributor

@emro emro commented Sep 28, 2020

Currently when you click on the sort buttons on the Valis browser, the position is reset to the default position. If the user has scrolled the browser to a new location, we want the browser to preserve this new location when the sort buttons are clicked.

@emro emro requested a review from zoldello September 28, 2020 17:03
Copy link
Contributor

@zoldello zoldello left a comment

Choose a reason for hiding this comment

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

This make sense. During sort, you save relevant parameter in the state. My comment is in the PR is not significant.

let x0 = this.state.x0;
let x1 = this.state.x1;
if (this.chartdisplay) {
const scrollLocation = this.chartdisplay.getElementsByClassName('hpgv_panel-header')[0].getElementsByTagName('span')[0].innerText;
Copy link
Contributor

Choose a reason for hiding this comment

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

The older means of referencing DOM like using "getElementsByClassName" is done when you need I speed. The speed difference is negligible unless you are using it in a loop with tens of thousands of iterations. Is there a special reason you are using it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that it would be better to use a ref? I think that could be tricky since I'm referring to an element generated in a component in a library. I thought about making a method to return the browser position but this was the simplest solution since it didn't require any changes to Valis and I think it should work consistently.

@@ -522,7 +522,22 @@ class GenomeBrowser extends React.Component {
if (files.length > 0) {
tracks = this.filesToTracks(newFiles, this.props.label, domain);
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it that this PR is part of an addition to a different ticket...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is the only change.

@gabdank gabdank merged commit e30528e into dev Oct 2, 2020
@gabdank gabdank deleted the ENCD-5537-save-browser-position-on-sort branch October 2, 2020 06:07
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.

3 participants