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

Checking that there is indeed a selection before scrolling #257

Merged
merged 11 commits into from
Feb 20, 2024
Merged

Checking that there is indeed a selection before scrolling #257

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4702db9
Checking that there is indeed a selection before scrolling
guzmanvig Jan 16, 2024
6700c6f
Run prettier
guzmanvig Jan 16, 2024
ae29dff
Merge branch 'develop' into scroll-to-selection
guzmanvig Jan 16, 2024
07ebdb7
Merge branch 'develop' into scroll-to-selection
jjti Feb 13, 2024
b984f49
Only scrolling if selection was done programatically
guzmanvig Feb 14, 2024
8d40d64
Linter run
guzmanvig Feb 14, 2024
3ad96e4
Implemented type guard for selection
guzmanvig Feb 14, 2024
6f8a4db
Updated comments
guzmanvig Feb 14, 2024
38b9f7d
Update src/SeqViewerContainer.tsx
guzmanvig Feb 14, 2024
b7ea117
Removed Selection type from props
guzmanvig Feb 14, 2024
3649075
Added ExternalSelection interface
guzmanvig Feb 14, 2024
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
42 changes: 32 additions & 10 deletions src/SeqViewerContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,13 @@ interface SeqViewerContainerProps {
rotateOnScroll: boolean;
search: NameRange[];
selectAllEvent: (event: React.KeyboardEvent<HTMLElement>) => boolean;
selection?: {
clockwise?: boolean;
end: number;
start: number;
};
selection?:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to change this one. This is the prop that can only be passed from the public prop. It's the state on the component, and the func args, that need that enhancement

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my understanding as well, but for some reason the functioncomponentDidUpdate = (prevProps: SeqViewerContainerProps) gets called when a user clicks on an element, and in this.props.selection you see the selection with type and the other fields.

In any case, I removed this since the type guard takes care of checking if it's a Selection or not.

| {
clockwise?: boolean;
end: number;
start: number;
}
| Selection;
seq: string;
seqType: SeqType;
showComplement: boolean;
Expand Down Expand Up @@ -91,6 +93,24 @@ class SeqViewerContainer extends React.Component<SeqViewerContainerProps, SeqVie
};
}

selectionIsProgramatic(selection: any): selection is Selection {
// If the selection was done programatically, it has not type
return !selection.type;
}

// If the selection prop updates, also scroll the linear view to the new selection
componentDidUpdate = (prevProps: SeqViewerContainerProps) => {
// Only scroll if the selection was done programatically
guzmanvig marked this conversation as resolved.
Show resolved Hide resolved
if (this.selectionIsProgramatic(this.props.selection)) {
if (
this.props.selection?.start !== prevProps.selection?.start &&
this.props.selection?.start !== this.props.selection?.end
) {
this.setCentralIndex("LINEAR", this.props.selection?.start || 0);
}
}
};

/** this is here because the size listener is returning a new "size" prop every time */
shouldComponentUpdate = (nextProps: SeqViewerContainerProps, nextState: any) =>
!isEqual(nextProps, this.props) || !isEqual(nextState, this.state);
Expand Down Expand Up @@ -125,11 +145,13 @@ class SeqViewerContainer extends React.Component<SeqViewerContainerProps, SeqVie
*/
getSelection = (
state: Selection,
prop?: {
clockwise?: boolean;
end: number;
start: number;
}
prop?:
| {
clockwise?: boolean;
end: number;
start: number;
}
| Selection
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't need to change either, it's 1:1 w/ the prop that users pass in so it doesn't have those other fields.

Looking thru it quickly, a few lines (maybe more) that need to change:

Copy link
Collaborator Author

@guzmanvig guzmanvig Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is right, the selection in the state has to be a Selection because it is then use everywhere else by passing it in <SelectionContext.Provider value={mergedSelection}>. And the getSelection function purpose is to convert the selection in props to a Selection:

  getSelection = (state: Selection, prop?: ExternalSelection): Selection => {
    if (prop) {
      return { ...prop, clockwise: typeof prop.clockwise === "undefined" || !!prop.clockwise, type: "" };
    }
    return state;
  };

I've added an ExternalSelection interface now to make it clearer, let me know what you think.

): Selection => {
if (prop) {
return { ...prop, clockwise: typeof prop.clockwise === "undefined" || !!prop.clockwise, type: "" };
Expand Down
Loading