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

Pangene Lookup Components #391

Merged
merged 35 commits into from
Oct 3, 2024
Merged

Pangene Lookup Components #391

merged 35 commits into from
Oct 3, 2024

Conversation

alancleary
Copy link
Contributor

This PR adds a pangene lookup component and various changes to support the component, including:

  • The form wrapper component now supports specifying which button to programmatically submit a form with
  • The paginated search mixin has been separated into two mixins: a basic search mixin and a paginated search mixin that extends the search mixin. Note: this is a breaking change as pagination parameters are now passed to the search function as part of the form data object instead of as separate parameters
  • The search mixin supports an optional download function that allows a component's data in its entirety (i.e. non-paginated) to be downloaded (downloads do not update querystring parameters)

This preserves some special characters that would otherwise be removed when the values are added to the querystring.
This allows errors that occurred during loading to be reported via the loading element's alert element.
… type

This allows errors to be reported even when there are partial results.
The element looks up pangenes for a list of gene identifiers that meet the given constraints.
Specifically, the search success and results info methods were updated to use generic types. This makes the these methods more reusable and independently extensible while making the generic search function and search result types more specific.
This means pagination information is no longer passed to the search function and results are not expected to include pagination information.
Except for annotation because these data are currently missing from the backend.
This label explains that the constraints apply to the genes being looked up, not the input genes.
This makes parsing the results much simpler and implementing pagination trivial.
This required adding genesRegexp and genesLimit properties to the component. These properties are used to implement a custom form field validator for the genes textarea. Lastly, the Lit willUpdate hook is used to decorate the searchFunction and downloadFunction properties when they are set so that the gene identifiers passed to these functions are split using the genesRegexp but the genes textarea text is encoded/decoded verbatim in the querystring parameters.
…vent

Specifically, the original submit event emitted by the wrapped form is now being included in the formEvent property of the custom event's detail object. This allows downstream event handlers to fully utilize the original event.
…apped search form

Specifically, the search mixin is using the original event to determine if the download button is what submitted the form.
The pagination component's page was mistakenly being reset to the default page (i.e. 1) for every search.
@alancleary alancleary added the enhancement New feature or request label Oct 3, 2024
@alancleary alancleary requested a review from adf-ncgr October 3, 2024 17:56
Copy link
Contributor

@adf-ncgr adf-ncgr left a comment

Choose a reason for hiding this comment

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

is there anything in particular you think I ought to look at? Otherwise I will just approve the changes, trusting that you know what you're doing here.

@alancleary
Copy link
Contributor Author

is there anything in particular you think I ought to look at? Otherwise I will just approve the changes, trusting that you know what you're doing here.

Mostly just one last kick of the tires. While preparing this PR I found a bit of code that hadn't been committed which is supposedly necessary so one last sanity check would be nice to be sure nothing else has slipped by.

Copy link
Contributor

@adf-ncgr adf-ncgr left a comment

Choose a reason for hiding this comment

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

One nitpick; can you change the verbiage for the case where the targeted annotations don't have any members in the pangeneset matched by the query gene? Currently given as "No matches found for identifier", we had discussed an alternative via email a while back. otherwise, seems to be working as I would expect.

@alancleary
Copy link
Contributor Author

One nitpick; can you change the verbiage for the case where the targeted annotations don't have any members in the pangeneset matched by the query gene? Currently given as "No matches found for identifier", we had discussed an alternative via email a while back. otherwise, seems to be working as I would expect.

Whoops, I forgot about that. In the email you suggested "no matching targets found in pangene set for gene x." How do you feel about the subtly different "no matching targets found in a pangene set for gene x"? The prior assumes each gene belongs to a single pangene set whereas the latter does not, which means that's one less thing to remember and fix if/when we end up having multiple pangene sets loaded simultaneously.

@alancleary alancleary merged commit 9a00311 into main Oct 3, 2024
1 check passed
@adf-ncgr
Copy link
Contributor

adf-ncgr commented Oct 3, 2024

sorry, was busy with other things. I feel fine about your subtle verbiage change and in fact tested the component against a version of glycinemine that has two pangenesets loaded concurrently- it works just as expected.

@alancleary
Copy link
Contributor Author

sorry, was busy with other things. I feel fine about your subtle verbiage change and in fact tested the component against a version of glycinemine that has two pangenesets loaded concurrently- it works just as expected.

The future is now I guess. Note that this error will only trigger if no matches are in full the result set, so if a query gene belongs to two pangene sets but only has matches for one of them then the error will not be shown. This scenario and the error verbiage are actually handled outside of the component so this is the sort of thing that can be fixed in the future without having to roll a new release of the components library.

@alancleary alancleary deleted the pangene-lookup branch October 10, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants