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

Orthogroup tree table protein filter #1252

Merged
merged 11 commits into from
Oct 31, 2024
Merged

Orthogroup tree table protein filter #1252

Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
355d5a4
the very basics
bobular Oct 28, 2024
f5ee686
straw man ready
bobular Oct 28, 2024
fccec4c
forgot the reset button
bobular Oct 28, 2024
ca400d5
DRYed up a bit
bobular Oct 28, 2024
ab88a2a
fixed the edge case as described in PR, improved search box responsiv…
bobular Oct 29, 2024
2f60de7
text search field selectors now more responsive
bobular Oct 29, 2024
72e4740
added asterisk and improved filter positioning
bobular Oct 31, 2024
c22b905
fixed broken logic with corePeripheral filter
bobular Oct 31, 2024
f6f9153
memoized rowsByAccession and mesaState and fixed some logic errors
bobular Oct 31, 2024
337348e
memoize treeProps just for tidiness
bobular Oct 31, 2024
2520735
replaced the key hack with ref approach
bobular Oct 31, 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
143 changes: 123 additions & 20 deletions packages/sites/ortho-site/webapp/wdkCustomization/js/client/records/Sequences.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import React, { CSSProperties, useCallback, useMemo, useState } from 'react';
import React, {
CSSProperties,
useCallback,
useDeferredValue,
useMemo,
useState,
} from 'react';
import TreeTable from '@veupathdb/components/lib/components/tidytree/TreeTable';
import { RecordTableProps, WrappedComponentProps } from './Types';
import { useOrthoService } from 'ortho-client/hooks/orthoService';
Expand All @@ -17,9 +23,12 @@ import { PfamDomainArchitecture } from 'ortho-client/components/pfam-domains/Pfa
import { extractPfamDomain } from 'ortho-client/records/utils';
import Banner from '@veupathdb/coreui/lib/components/banners/Banner';
import { RowCounter } from '@veupathdb/coreui/lib/components/Mesa';
import PopoverButton from '@veupathdb/coreui/lib/components/buttons/PopoverButton/PopoverButton';
import { PfamDomain } from 'ortho-client/components/pfam-domains/PfamDomain';
import {
FilledButton,
FloatingButton,
OutlinedButton,
SelectList,
Undo,
useDeferredState,
Expand All @@ -45,13 +54,15 @@ export function RecordTable_Sequences(
props: WrappedComponentProps<RecordTableProps>
) {
const [searchQuery, setSearchQuery] = useState('');
const safeSearchRegexp = useMemo(
() => createSafeSearchRegExp(searchQuery),
[searchQuery]
const safeSearchRegexp = useDeferredValue(
useMemo(() => createSafeSearchRegExp(searchQuery), [searchQuery])
);

const [resetCounter, setResetCounter] = useState(0); // used for forcing re-render of filter buttons

const [proteinFilterIds, setProteinFilterIds, volatileProteinFilterIds] =
useDeferredState<string[]>([]);

const [selectedSpecies, setSelectedSpecies, volatileSelectedSpecies] =
useDeferredState<string[]>([]);

Expand Down Expand Up @@ -185,13 +196,15 @@ export function RecordTable_Sequences(
// 2. core-peripheral radio button
// 3. checked boxes in the Pfam legend

const [selectedColumnFilters, setSelectedColumnFilters] = useState<string[]>(
[]
);
const [
selectedColumnFilters,
setSelectedColumnFilters,
volatileSelectedColumnFilters,
] = useDeferredState<string[]>([]);

const filteredRows = useMemo(() => {
if (
searchQuery !== '' ||
safeSearchRegexp != null ||
corePeripheralFilterValue != null ||
pfamFilterIds.length > 0 ||
selectedSpecies.length > 0
Expand All @@ -204,7 +217,7 @@ export function RecordTable_Sequences(
const rowPfamIdsSet = accessionToPfamIds.get(rowFullId);

const searchMatch =
searchQuery === '' ||
safeSearchRegexp == null ||
rowMatch(row, safeSearchRegexp, selectedColumnFilters);
const corePeripheralMatch =
corePeripheralFilterValue.length === 0 ||
Expand All @@ -217,22 +230,29 @@ export function RecordTable_Sequences(
const speciesMatch =
selectedSpecies.length === 0 ||
selectedSpecies.some((specie) => row.taxon_abbrev === specie);
const proteinMatch =
proteinFilterIds.length === 0 ||
proteinFilterIds.some((proteinId) => rowFullId === proteinId);

return (
searchMatch && corePeripheralMatch && pfamIdMatch && speciesMatch
searchMatch &&
corePeripheralMatch &&
pfamIdMatch &&
speciesMatch &&
proteinMatch
);
});
}
return undefined;
}, [
searchQuery,
selectedColumnFilters,
safeSearchRegexp,
sortedRows,
corePeripheralFilterValue,
accessionToPfamIds,
pfamFilterIds,
selectedSpecies,
proteinFilterIds,
]);

// now filter the tree if needed - takes a couple of seconds for large trees
Expand Down Expand Up @@ -303,12 +323,7 @@ export function RecordTable_Sequences(
numSequences <= MAX_SEQUENCES_FOR_TREE &&
(tree == null || treeResponse == null))
) {
return (
<>
<div>Loading...</div>
<Loading />
</>
); // The loading spinner does not show :-(
return <Loading />;
}

if (
Expand Down Expand Up @@ -457,6 +472,90 @@ export function RecordTable_Sequences(
/>
) : null;

const resetProteinFilterButton = (
<OutlinedButton
text="Reset protein filter"
onPress={() => {
setProteinFilterIds([]);
}}
/>
);

const updateProteinFilterIds = () => {
setProteinFilterIds(highlightedNodes);
setHighlightedNodes([]);
};

const proteinFilter = (
<PopoverButton
buttonDisplayContent={`Proteins${
volatileProteinFilterIds.length > 0
? ` (${volatileProteinFilterIds.length})`
: ''
}`}
key={volatileProteinFilterIds.join(':')}
Copy link
Member

Choose a reason for hiding this comment

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

Should we change the label when rows are selected, to indicate that the underlying filter is operable?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an asterisk just as a placeholder (though the asterisk is also used inside the popover, so maybe it's good)

>
Copy link
Member

Choose a reason for hiding this comment

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

This isn't needed

Suggested change
key={volatileProteinFilterIds.join(':')}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is necessary - or if we remove it we need another way to close the popover after an action button is clicked

image

Copy link
Member

Choose a reason for hiding this comment

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

I see. This could use a comment.

I'm not a big fan of using keys like this, but it does solve this problem.

There are two ways we can avoid using key, here:

  1. Have PopoverButton take an open prop that can be used to control the internal state.
  2. Have PopoverButton take a ref that exposes a close() and an open() method.

Both have their pros and cons. The first option means introducing more state for the consumer, and more state management for the PopoverButton component, but it uses props, which is more common. The latter doesn't not introduce new state and works better with event handlers, but custom refs are not nearly as common to work with.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't expose an open() method just yet. We can grow into that!

<div
style={{
margin: '1em',
display: 'flex',
flexDirection: 'column',
gap: '1em',
maxWidth: '300px',
}}
>
{highlightedNodes.length === 0 ? (
volatileProteinFilterIds.length === 0 ? (
<div>
Select some proteins using the checkboxes in the table below.
</div>
) : (
<>
<div>
You are filtering on{' '}
{volatileProteinFilterIds.length.toLocaleString()} proteins.
</div>
{resetProteinFilterButton}
</>
)
) : volatileProteinFilterIds.length === 0 ? (
<>
<div>
You have checked {highlightedNodes.length.toLocaleString()}{' '}
proteins in the table.
</div>
<FilledButton
text="Filter to keep only these proteins"
onPress={updateProteinFilterIds}
/>
</>
) : highlightedNodes.length < volatileProteinFilterIds.length ? (
<>
<div>
You have checked {highlightedNodes.length.toLocaleString()}{' '}
proteins in the table and are already filtering on{' '}
{volatileProteinFilterIds.length.toLocaleString()} proteins.
</div>
<FilledButton
text="Refine filter to keep only checked proteins"
onPress={updateProteinFilterIds}
/>
{resetProteinFilterButton}
</>
) : (
<>
<div>
You have checked all the proteins that are currently being
filtered on. Either uncheck one or more proteins or reset the
filter entirely using the button below.
</div>
{resetProteinFilterButton}
</>
)}
</div>
</PopoverButton>
);

const resetButton = (
<FloatingButton
text={''}
Expand All @@ -465,13 +564,15 @@ export function RecordTable_Sequences(
disabled={
pfamFilterIds.length +
corePeripheralFilterValue.length +
selectedSpecies.length ===
selectedSpecies.length +
proteinFilterIds.length ===
0
}
icon={Undo}
size={'medium'}
themeRole={'primary'}
onPress={() => {
setProteinFilterIds([]);
setPfamFilterIds([]);
setCorePeripheralFilterValue([]);
setSelectedSpecies([]);
Expand Down Expand Up @@ -527,7 +628,7 @@ export function RecordTable_Sequences(
onSearchTermChange={setSearchQuery}
recordDisplayName="Proteins"
filterAttributes={filterAttributes}
selectedColumnFilters={selectedColumnFilters}
selectedColumnFilters={volatileSelectedColumnFilters}
onColumnFilterChange={(keys) => setSelectedColumnFilters(keys)}
/>
<div className="MesaComponent" style={{ marginRight: 'auto' }}>
Expand All @@ -551,6 +652,7 @@ export function RecordTable_Sequences(
}}
>
<strong>Filters: </strong>
{proteinFilter}
{pfamFilter}
{corePeripheralFilter}
{taxonFilter}
Expand Down Expand Up @@ -639,7 +741,8 @@ function rowMatch(row: RowType, query: RegExp, keys?: string[]): boolean {
);
}

function createSafeSearchRegExp(input: string): RegExp {
function createSafeSearchRegExp(input: string): RegExp | undefined {
if (input === '') return undefined;
try {
// Attempt to create a RegExp from the user input directly
return new RegExp(input, 'i');
Expand Down
Loading