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.

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

Conversation

bobular
Copy link
Member

@bobular bobular commented Oct 28, 2024

There's an edge case where say you are filtered on 6 proteins and you select-all 6 proteins and then click "refine filter" the filter widget doesn't close automatically (because our key doesn't change) Solved ✔️

@bobular bobular requested a review from dmfalke October 28, 2024 18:46
@bobular bobular linked an issue Oct 28, 2024 that may be closed by this pull request
@bobular
Copy link
Member Author

bobular commented Oct 29, 2024

There's something strange about the selectedColumnFilters (table text search drop-down field selector)

When you have a large group (OG7_0000011) and no text search at all, it takes around a second for the checkbox clicks to register.

When you have a search active (e.g. "123") it's more snappy.

I cannot see why!

@bobular
Copy link
Member Author

bobular commented Oct 29, 2024

Note: there seems to be no urgent need to address the debouncing quirks in RealTimeSearchBox (unused delayMs prop) - performance seems OK as-is.

Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

Overall, this is a good straw man. I anticipate lots of opinions and potential changes, so this is a really good and simple starting point.

I did make a couple of suggestions (see below).

? ` (${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.

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!

Comment on lines 491 to 495
buttonDisplayContent={`Proteins${
volatileProteinFilterIds.length > 0
? ` (${volatileProteinFilterIds.length})`
: ''
}`}
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)

@bobular bobular merged commit 07a9a6d into orthogroup-tree-table Oct 31, 2024
1 check passed
@bobular bobular deleted the orthogroup-tree-table__protein-filter branch October 31, 2024 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ortho 7 - implement protein filter
2 participants