-
Notifications
You must be signed in to change notification settings - Fork 168
Reset layout when nucleotide position is selected #1947
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
base: master
Are you sure you want to change the base?
Conversation
Previously, this did not work because the dispatch to reset layout was conditional on having a CDS selected.
| if (arg === nucleotide_gene) { | ||
| if (entropy.selectedCds === nucleotide_gene) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you've highlighted a nucleotide bar (in the entropy panel) and click "reset layout" the bar will no longer be highlighted because now action.selectedPositions = []; will be hit below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply dropping action.selectedPositions = [] doesn't work, I think because zoom bounds are tightly coupled with selected positions here:
auspice/src/components/entropy/entropyD3.js
Lines 131 to 145 in 1f40cd0
| if (this.selectedPositions?.length) { | |
| const posMin = Math.min(...this.selectedPositions); | |
| const posMax = Math.max(...this.selectedPositions); | |
| /* If we already have coordinates which are not the default, and the new | |
| positions are within the coordinates, then don't change zoom at all */ | |
| if ( | |
| this.zoomCoordinates && | |
| (this.zoomCoordinates[0]!==domain[0] || this.zoomCoordinates[1]!==domain[1]) && | |
| posMin>this.zoomCoordinates[0] && posMax<this.zoomCoordinates[1] | |
| ) { | |
| return; | |
| } | |
| let desiredSurroundingSpace = Math.floor(domain[1] * 0.05); // 5% | |
| if (desiredSurroundingSpace>1000) desiredSurroundingSpace=1000; // up to a max of 1kb | |
| this.zoomCoordinates = [posMin - desiredSurroundingSpace, posMax + desiredSurroundingSpace]; |
| } else { | ||
| this.props.dispatch(changeEntropyCdsSelection(nucleotide_gene)); | ||
| } | ||
| this.props.dispatch(changeEntropyCdsSelection(nucleotide_gene)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change (and the earlier change), the following just above this line becomes unnecessary and can be removed:
if (viewingGenome) {
this.state.chart.update({
zoomMin: this.state.chart.zoomBounds[0],
zoomMax: this.state.chart.zoomBounds[1],
})
}This is because we end up running update({selectedPositions: []}) which runs
if (selectedCds || selectedPositions !== undefined) {
this._setZoomCoordinates(zoomMin, zoomMax, !!selectedCds);
}so we'll always reset the coordinates to show the entire nuc span.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still used for when no CDS or position is selected, when the min/max sliders are adjusted. I think the update can happen elsewhere though. Started with 1f40cd0 but haven't gotten it fully working.
Description of proposed changes
Previously, this did not work because the dispatch to reset layout was conditional on having a CDS selected.
Related issue(s)
Closes #1774
Checklist