Skip to content

Conditionally disable reset layout button #1948

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

Open
victorlin wants to merge 3 commits into
base: victorlin/fix-entropy-reset
Choose a base branch
from
Open

Conditionally disable reset layout button #1948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5ba1cf
Keep entropy zoom state props in sync with URL params
victorlin Mar 28, 2025
b710514
Align styling with tree panel's reset layout button
victorlin Mar 28, 2025
e23e929
Conditionally disable reset layout button
victorlin Mar 28, 2025
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
141 changes: 73 additions & 68 deletions src/components/entropy/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { FaInfoCircle } from "react-icons/fa";
import { isEqual } from 'lodash';
import Card from "../framework/card";
import { changeColorBy } from "../../actions/colors";
import { tabGroup, tabGroupMember, tabGroupMemberSelected } from "../../globalStyles";
import { tabGroup, tabGroupMember, tabGroupMemberSelected, tabSingle, darkGrey, lightGrey } from "../../globalStyles";
import EntropyChart from "./entropyD3";
import InfoPanel from "./infoPanel";
import { changeEntropyCdsSelection, showCountsNotEntropy } from "../../actions/entropy";
Expand All @@ -21,46 +21,6 @@ import { StyledTooltip } from "../controls/styles";
import "../../css/entropy.css";


const getStyles = (width) => {
return {
switchContainer: {
position: "absolute",
marginTop: -5,
paddingLeft: width - 100
},
switchContainerWide: {
position: "absolute",
marginTop: -25,
paddingLeft: width - 185
},
switchTitle: {
margin: 5,
position: "relative",
top: -1
},
resetLayout: {
position: "absolute",
right: 190,
top: 0,
zIndex: 100
},
entropyCountSwitch: {
position: "absolute",
right: 50,
top: 0,
zIndex: 100
},
helpIcon: {
position: "absolute",
right: 25,
top: 3,
fontSize: '16px', // controls icon size
cursor: 'help',
color: '#888'
}
};
};

@connect((state) => {
return {
selectedCds: state.entropy.selectedCds,
Expand All @@ -72,11 +32,6 @@ const getStyles = (width) => {
loaded: state.entropy.loaded,
onScreen: state.entropy.onScreen,
colorBy: state.controls.colorBy,
/**
* Note that zoomMin & zoomMax only represent the state when changed by a URL
* i.e. on dataset load or narrative page change. As such, they fall out-of-sync
* as soon as any user-zooming is performed.
*/
zoomMin: state.controls.zoomMin,
zoomMax: state.controls.zoomMax,
defaultColorBy: state.controls.defaults.colorBy,
Expand Down Expand Up @@ -131,29 +86,27 @@ class Entropy extends React.Component {
const viewingGenome = this.props.selectedCds===nucleotide_gene;
/**
* The intention for this button is to be inactive when viewing the genome &
* fully zoomed out, however zoom actions do not trigger redux state changes
* which would be necessary for this (see comment in @connect decorator
* above). Once we fix that it is simple to conditionally inactivate this
* button.
* fully zoomed out.
*/
return (
<div style={{...tabGroup, ...styles.resetLayout}}>
<button
key={1}
style={tabGroupMember}
onClick={() => {
if (viewingGenome) {
this.state.chart.update({
zoomMin: this.state.chart.zoomBounds[0],
zoomMax: this.state.chart.zoomBounds[1],
})
}
this.props.dispatch(changeEntropyCdsSelection(nucleotide_gene));
}}
>
<span style={styles.switchTitle}> {'RESET LAYOUT'} </span>
</button>
</div>
<button
key={1}
style={{...tabSingle, ...styles.resetLayout}}
onClick={() => {
if (!this.isZoomed()) {
return;
}
if (viewingGenome) {
this.state.chart.update({
zoomMin: this.state.chart.zoomBounds[0],
zoomMax: this.state.chart.zoomBounds[1],
})
}
this.props.dispatch(changeEntropyCdsSelection(nucleotide_gene));
}}
>
<span style={styles.switchTitle}> {'RESET LAYOUT'} </span>
</button>
);
}
entropyCountSwitch(styles) {
Expand Down Expand Up @@ -276,8 +229,60 @@ class Entropy extends React.Component {
return `Amino acid diversity of CDS ${this.props.selectedCds.name}`
}

getStyles() {
const zoomed = this.isZoomed();

return {
switchContainer: {
position: "absolute",
marginTop: -5,
paddingLeft: this.props.width - 100
},
switchContainerWide: {
position: "absolute",
marginTop: -25,
paddingLeft: this.props.width - 185
},
switchTitle: {
margin: 5,
position: "relative",
top: -1
},
resetLayout: {
position: "absolute",
right: 190,
top: 0,
zIndex: 100,
cursor: zoomed ? "pointer" : "auto",
color: zoomed ? darkGrey : lightGrey
},
entropyCountSwitch: {
position: "absolute",
right: 50,
top: 0,
zIndex: 100
},
helpIcon: {
position: "absolute",
right: 25,
top: 3,
fontSize: '16px', // controls icon size
cursor: 'help',
color: '#888'
}
};
}

isZoomed() {
return (
(this.props.selectedCds !== nucleotide_gene) ||
(this.state.chart.zoomBounds && this.props.zoomMin !== this.state.chart.zoomBounds[0]) ||
(this.state.chart.zoomBounds && this.props.zoomMax !== this.state.chart.zoomBounds[1])
);
}

render() {
const styles = getStyles(this.props.width);
const styles = this.getStyles();
return (
<Card infocard={this.props.showOnlyPanels} title={this.title()}>
<InfoPanel d3event={this.state.hovered.d3event} width={this.props.width} height={this.props.height}>
Expand Down
11 changes: 2 additions & 9 deletions src/reducers/controls.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,15 +512,8 @@ const Controls = (state: ControlsState = getDefaultControlsState(), action): Con
case types.TOGGLE_MEASUREMENTS_THRESHOLD: // fallthrough
case types.APPLY_MEASUREMENTS_FILTER:
return {...state, ...action.controls};
/**
* Currently the CHANGE_ZOOM action (entropy panel zoom changed) does not
* update the zoomMin/zoomMax, and as such they only represent the initially
* requested zoom range. The following commented out code will keep the
* state in sync, but corresponding changes will be required to the entropy
* code.
*/
// case types.CHANGE_ZOOM: // this is the entropy panel zoom
// return {...state, zoomMin: action.zoomc[0], zoomMax: action.zoomc[1]};
case types.CHANGE_ZOOM: // this is the entropy panel zoom
return {...state, zoomMin: action.zoomc[0], zoomMax: action.zoomc[1]};
Comment on lines +515 to +516
Copy link
Member

@jameshadfield jameshadfield Mar 31, 2025

Choose a reason for hiding this comment

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

I remember disabling this as it caused bugs, or maybe unnecessary renders, but I can't remember the specifics and 2a063a7 doesn't spell them out either. I'll do some more testing when the PR's ready to go!

Copy link
Member

@jameshadfield jameshadfield Mar 31, 2025

Choose a reason for hiding this comment

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

or maybe unnecessary renders

This is it I think. With these changes we now trigger two calls to update rather than one. The first has no zoom information, the second sets min/max to the overall zoom bounds. (Sometimes three calls, but my suggested changes in #1947 will limit it to two calls.)

I don't think this is blocking, although would be worth spending a small amount of time to see if the logic can be improved so there's only one update call.

default:
return state;
}
Expand Down