Orthogroup tree table 1254 fixes #1259

bobular · 2024-11-01T18:05:12Z

Resolves #1254

I looked into improving the responsiveness of the table paging controls with useDeferredState, but it was not possible due to the memoisation of mesaState - which has a mix of "slow" and "volatile" dependencies. I wonder if that memoisation isn't actually necessary? (Answer: I removed it and this reintroduced the janky search field checkboxes...)

…ogroup-tree-table__1254-fixes

dmfalke

I think this is mostly great. I made several comments, but I don't think any are show stoppers.

dmfalke · 2024-11-04T15:00:05Z

packages/libs/coreui/src/components/inputs/SelectList.tsx

@@ -15,7 +18,7 @@ export interface SelectListProps<T> extends CheckboxListProps<T> {
  instantUpdate?: boolean;
 }

-export default function SelectList<T>({
+export default function SelectList<T extends ReactNode>({


This seems strange to me. T is the type of the value. I don't think we ever want that to be a ReactNode. I think we should have T extends string, to be honest.

The change you made for buttonDisplayContent is perfect.

Thanks. I'll try that, but it was my understanding that T needed to be something "compatible with" ReactNode.

I think of value as what would be included in a form request, which is always a string. A number could also work, but I'm not convinced we need to do that.

This would be a breaking change, but I'm not sure if it would actually break anything.

I've fixed it as you suggested*. If it's a breaking change, it hasn't broken anything within this repo at least!

I messed up the commits and it went in as two different commits, but the commit messages hopefully explain all.

dmfalke · 2024-11-04T15:00:46Z

packages/sites/ortho-site/webapp/wdkCustomization/js/client/records/Sequences.tsx

+import {
+  areTermsInStringRegexString,
+  parseSearchQueryString,
+} from '../../../../../../../libs/wdk-client/lib/Utils/SearchUtils';


Suggested change

} from '../../../../../../../libs/wdk-client/lib/Utils/SearchUtils';

} from '@veupathdb/wdk-client/lib/Utils/SearchUtils';

dmfalke · 2024-11-04T15:06:22Z

packages/sites/ortho-site/webapp/wdkCustomization/js/client/records/Sequences.tsx

+    (mesaRows.length !== sortedRows.length ||
+      mesaRows.length !== leaves?.length)


Are both comparisons necessary?

Yes, I think so.

sortRows() returns the protein table rows in tree leaf order. Proteins that are missing from the tree are an obvious problem. But the tree can also be bigger (more leaves) than the protein table (though Rich says it won't happen) - it's best to play on the safe side.

Just using mesaRows.length !== leaves?.length won't protect against a protein table of X rows and a tree with X leaves, but where none of the proteins/leaves match each other.

dmfalke · 2024-11-04T15:07:03Z

packages/sites/ortho-site/webapp/wdkCustomization/js/client/records/Sequences.tsx

+            <span>
+              A data processing error has occurred on our end. We apologize for
+              the inconvenience. If this problem persists, please{' '}
+              <Link to="/contact-us">contact us</Link>.


Maybe add target="_blank"

dmfalke · 2024-11-04T15:10:05Z

packages/sites/ortho-site/webapp/wdkCustomization/js/client/records/Sequences.tsx

+  const warningText =
+    filteredRows.length > MAX_SEQUENCES_FOR_TREE ||
+    filteredRows.length < MIN_SEQUENCES_FOR_TREE ? (
+      <span>
+        To see a phylogenetic tree please use a filter to display between{' '}
+        {MIN_SEQUENCES_FOR_TREE.toLocaleString()} and{' '}
+        {MAX_SEQUENCES_FOR_TREE.toLocaleString()} sequences
+      </span>
+    ) : filteredRows.length < sortedRows.length ? (
+      <span>
+        Note: The ortholog group's phylogeny has been pruned to display only the
+        currently filtered proteins. This may differ from a tree constructed{' '}
+        <i>de novo</i> using only these sequences.
+      </span>
+    ) : undefined;
+


I have a concern about putting this new message in the same place as the other message. It gets lost. Wouldn't it make more sense to add this to the table description where we describe the ability to filter?

You mean it gets lost because it's the same colour and position as the "too many proteins" message that was almost certainly there just before they filtered?

I'm not going to tackle this tonight. I don't really see it as a problem, personally. But we can discuss tomorrow.

You mean it gets lost because it's the same colour and position as the "too many proteins" message that was almost certainly there just before they filtered?

Yes, that was my thinking. Let's leave it as-is and see if others raise it as an issue.

…ogroup-tree-table__1254-fixes

…boxList/SelectList generics 1 of 2

… now it isn't

bobular · 2024-11-05T23:22:06Z

I'd be grateful if you could have a quick look at the latest changes, @dmfalke - thanks!

dmfalke

This looks great. I made one last suggestion. There may be a couple of other places that we should use MIN_SEQUENCES_FOR_TREE. Otherwise, I think this is good to go.

dmfalke · 2024-11-06T14:27:06Z

packages/sites/ortho-site/webapp/wdkCustomization/js/client/records/Sequences.tsx

@@ -98,7 +98,10 @@ export function RecordTable_Sequences(
  const numSequences = mesaRows.length;

  const treeResponse = useOrthoService(
-    (orthoService) => orthoService.getGroupTree(groupName),
+    (orthoService) => {
+      if (numSequences < 3) return Promise.resolve(undefined);


Should probably use MIN_SEQUENCES_FOR_TREE here.

bobular added 7 commits November 1, 2024 17:49

new warning added, no new component, sorry

5e04a76

improved tree error warning with link to contact form

bbd65ea

use display names in SelectList button labels

564f453

standardised search behaviour with help text

6907790

Merge remote-tracking branch 'origin/orthogroup-tree-table' into orth…

494e55d

…ogroup-tree-table__1254-fixes

protein filter and paged table interaction fixed

25def36

fixed label display issue in EDA

075fff3

bobular linked an issue Nov 2, 2024 that may be closed by this pull request

Ortho 7 - minor UX upgrades #1254

Open

8 tasks

bobular marked this pull request as ready for review November 2, 2024 11:31

bobular requested a review from dmfalke November 2, 2024 11:31

dmfalke reviewed Nov 4, 2024

View reviewed changes

bobular added 7 commits November 5, 2024 20:53

Merge remote-tracking branch 'origin/orthogroup-tree-table' into orth…

0c382ac

…ogroup-tree-table__1254-fixes

target _blank

997837d

fix import path

2e76120

show table for 1 or 2 protein groups, not the error. Changes to Check…

6b8f7f4

…boxList/SelectList generics 1 of 2

changes to CheckboxList/SelectList generics 2 of 2

5deced2

reset button also cancels text search

4557b84

error banner was showing instead of loading spinner for large groups,…

2fa75aa

… now it isn't

bobular requested a review from dmfalke November 5, 2024 23:21

dmfalke approved these changes Nov 6, 2024

View reviewed changes

use const for min seqs in tree

7ef7b0e

bobular merged commit d24c342 into orthogroup-tree-table Nov 6, 2024
1 check passed

bobular deleted the orthogroup-tree-table__1254-fixes branch November 6, 2024 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orthogroup tree table 1254 fixes #1259

Orthogroup tree table 1254 fixes #1259

bobular commented Nov 1, 2024 •

edited

Loading

dmfalke left a comment

dmfalke Nov 4, 2024

bobular Nov 5, 2024

dmfalke Nov 5, 2024

bobular Nov 5, 2024 •

edited

Loading

dmfalke Nov 4, 2024

dmfalke Nov 4, 2024

bobular Nov 5, 2024

dmfalke Nov 4, 2024

dmfalke Nov 4, 2024

bobular Nov 5, 2024

bobular Nov 5, 2024

dmfalke Nov 6, 2024

bobular commented Nov 5, 2024

dmfalke left a comment

dmfalke Nov 6, 2024

	} from '../../../../../../../libs/wdk-client/lib/Utils/SearchUtils';
	} from '@veupathdb/wdk-client/lib/Utils/SearchUtils';

		(mesaRows.length !== sortedRows.length \|\|
		mesaRows.length !== leaves?.length)

Orthogroup tree table 1254 fixes #1259

Orthogroup tree table 1254 fixes #1259

Conversation

bobular commented Nov 1, 2024 • edited Loading

dmfalke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobular Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobular commented Nov 5, 2024

dmfalke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobular commented Nov 1, 2024 •

edited

Loading

bobular Nov 5, 2024 •

edited

Loading