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

NickAkhmetov/HMP-500 - Biomarkers landing page #3330

Merged
merged 31 commits into from
Dec 1, 2023

Conversation

NickAkhmetov
Copy link
Collaborator

@NickAkhmetov NickAkhmetov commented Nov 15, 2023

This PR

  • Implements the Biomarkers landing page, which currently displays only genes
    • I also implemented a basic mobile view
  • Adds a link to the Biomarkers landing page to the Other dropdown in the site header
  • Converts panel lists to TSX and extends functionality to simplify creation of customized panels going forward
2023-11-30.biomarker.landing.page.demo.1.mov

Base automatically changed from nickakhmetov/hmp-495-gene-details-cells-api to main November 16, 2023 19:58
@NickAkhmetov NickAkhmetov marked this pull request as ready for review November 30, 2023 20:32
@tsliaw
Copy link
Contributor

tsliaw commented Dec 1, 2023

@NickAkhmetov Can you clarify to me how the search works? Is it only looking up by gene symbols right now and is case-sensitive? Because anytime I am typing anything that is lower-case, there are no results.

If that is how the search works, this isn't blocking. I just need to know so I know how to change the language of the placeholder text.

Screenshot 2023-12-01 at 10 42 48 AM
Screenshot 2023-12-01 at 10 43 20 AM

@NickAkhmetov
Copy link
Collaborator Author

@tsliaw Unfortunately, search is case sensitive and matches against the start of a string gene name/alias/symbol.

Per Alan, the reason for case sensitivity is due to the endpoint matching against a variety of identifiers, including name and alias: https://hubmapconsortium.slack.com/archives/C05CYU38D7T/p1699019370903709

We should definitely adjust the current text to reflect this limitation.

@tsliaw
Copy link
Contributor

tsliaw commented Dec 1, 2023

@NickAkhmetov Is it also matching with gene name or just with symbol? Since it is case-sensitive, I would expect the lower-case searches to match with some of the gene names.

return (
<Stack direction="column" spacing={1}>
<div>
Gene and protein information are available in HuBMAP data. To look up information about a specific gene or
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the text to
Gene information is currently available in HuBMAP data. To look up information about a specific gene, use the search bar below. Protein information is upcoming. Launch the <InternalLink href="/cells">Molecular Query</InternalLink> to find datasets for a specific biomarker.

We can revert back to the original text once we have proteins in place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍🏻 Done!

import { useBiomarkersSearchActions, useBiomarkersSearchState } from './BiomarkersSearchContext';

const searchbarPlaceholder =
'Search for biomarkers by gene symbol, gene name or protein name. Example: CD4, Cytokeratin';
Copy link
Contributor

Choose a reason for hiding this comment

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

If search is only gene symbol, change text to
Search for biomarkers by gene symbol. Note that searches are case-sensitive. Example: CD4, MMRN1

If search is both gene name and symbol, change text to
Search for biomarkers by gene name or symbol. Note that searches are case-sensitive. Example: CD4, MMRN1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pending a response from Alan on this, but went with the former for now since that's the behavior we've personally observed

@NickAkhmetov
Copy link
Collaborator Author

@NickAkhmetov Is it also matching with gene name or just with symbol? Since it is case-sensitive, I would expect the lower-case searches to match with some of the gene names.

According to Alan in the conversation I linked, it should be matching on names as well, but I haven't observed any results for lowercase queries. We should confirm with him.

john-conroy
john-conroy previously approved these changes Dec 1, 2023
Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

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

Nice! Very minor points feel free to ignore. We should all discuss a media query strategy moving forward.

Comment on lines 18 to 23
const responsiveProps: Partial<StackProps> = {
height: isMobile ? 'unset' : 52,
direction: isMobile ? 'column' : 'row',
spacing: isMobile ? 2 : 4,
py: isMobile ? 2 : 0,
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer having a single ternary for readability, but not major.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pulled the two constants out of the component and made this a ternary between them instead, good call!

Comment on lines 58 to 90
const propsList: PanelProps[] = [
{
key: 'filters',
noHover: true,
children: <BiomarkerPanel.Filters />,
},
{
key: 'header',
noPadding: true,
children: <BiomarkerPanel.Header />,
},
];
propsList.push(
...genesList.map(({ approved_name, approved_symbol, summary }) => ({
key: approved_symbol,
noPadding: true,
children: (
<BiomarkerPanel.Item
name={`${approved_name} (${approved_symbol})`}
description={summary || 'No description available.'}
href={`/genes/${approved_symbol}`}
type="Gene"
/>
),
})),
);
propsList.push({
key: 'view-more',
noPadding: true,
children: <ViewMoreButton />,
});
return propsList;
}, [genesList, isLoading]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I'm missing something, but why push? Can these not just be defined in propsList?

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's an artifact of a previous approach, good catch. I was originally unsure whether to display the filters/header when no results were available, but I decided against that for simplicity.


const genesList = data?.flatMap((page) => page.genes) ?? [];

const hasMore = data?.[0]?.pagination?.page !== data?.[0]?.pagination?.total_pages;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could avoid repeating data?.[0].pagination.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call; I've also revised this to use .at(-1) instead of [0] to make sure the latest page's number is used.

Comment on lines 18 to 24
export function useViewMore() {
const { size, setSize, isLoading, isValidating } = useResultsList();
return useCallback(async () => {
if (isLoading || isValidating) return;
await setSize(size + 1);
}, [isLoading, isValidating, setSize, size]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could move into the useResultsList hook for simplicity since it only depends on values returned by that hook?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Realistically, I should be using useGenesList here instead; both useResultsList and useViewMore are going to grow in complexity as we introduce proteins to this page, so having these hooks be decoupled from one another will be useful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a local useCurrentGenesList hook to avoid repeatedly needing to access the context to pull out the search value to provide to useGenesList which the useResultsList and useViewMore hooks can both use to access the genes SWR data.

john-conroy
john-conroy previously approved these changes Dec 1, 2023
Copy link
Collaborator

@john-conroy john-conroy left a comment

Choose a reason for hiding this comment

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

Thanks!

@NickAkhmetov NickAkhmetov merged commit c6c116f into main Dec 1, 2023
8 checks passed
@NickAkhmetov NickAkhmetov deleted the nickakhmetov/hmp-500-biomarker-landing-page branch December 1, 2023 22:08
lchoy pushed a commit that referenced this pull request Dec 13, 2023
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.

3 participants