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

John conroy/replace searchkit #3441

Open
wants to merge 77 commits into
base: main
Choose a base branch
from
Open

Conversation

john-conroy
Copy link
Collaborator

Feature branch for the effort to replace searchkit.

@john-conroy john-conroy marked this pull request as ready for review October 17, 2024 16:45
Copy link
Collaborator

@NickAkhmetov NickAkhmetov left a comment

Choose a reason for hiding this comment

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

Great work getting through this gargantuan effort 🙌🏻

I went through the search pages on local. My observations are noted below; I've also gone through the code and made suggestions where things stood out to me. I anticipate making a few more passes through the code as we iterate.

Datasets

Screen.Recording.2024-10-17.134635.mp4
  • It might be helpful to show a loading indicator in the show more button after it's clicked in case the user has a slow connection.
  • The page seems to hang for a second or two when filters are changed:
Screen.Recording.2024-10-17.135128.mp4
  • Expanding/collapsing a search facet without changing any filters appears to change the displayed search results - this may be related to the issue above.
Screen.Recording.2024-10-17.135251.mp4
  • Somewhat funny example of the same issue, the order of two results changes when I expand/collapse the group:
Screen.Recording.2024-10-17.140340.mp4

Donors

  • Donor page results' column sizing is a bit off - the same amount of space is allotted to each column, despite Group being a much more text-heavy field than Donor BMI or Donor Age. The donor age tooltips from Austenem/CAT-873 add donor age messaging #3548 are also missing.
    image

  • Related to the above, Donor Age slider should be capped at 90; the visualization also appears somewhat misaligned, since the last bin in the histogram lines up with 80.
    image

  • Also, should we consider naming the columns "Age" and "BMI" to be a bit less redundant?

Publications

The chronological sort order applied in #3565 is no longer present/the publications list is no longer sorted:
image

Organ pages

  • Search page links from assay graphs need to be updated to use the new buildSearchLink util:
    image

Homepage

Same as the organ page, the graph's links need to be updated to use the new util.
image

The collections count fix from #3564 should be restored:
image

Comment on lines +12 to +15
const FacetAccordionDetails = styled(AccordionDetails)({
flexDirection: 'column',
padding: '4px 4px 4px 8px',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I figure we can use theme.spacing here instead:

Suggested change
const FacetAccordionDetails = styled(AccordionDetails)({
flexDirection: 'column',
padding: '4px 4px 4px 8px',
});
const FacetAccordionDetails = styled(AccordionDetails)(({ theme }) => ({
flexDirection: 'column',
padding: theme.spacing(0.5, 0.5, 0.5, 1),
}));

Comment on lines +17 to +18
const FacetAccordionSummary = styled(AccordionSummary)(({ $position }: { $position: 'inner' | 'outer' }) => ({
padding: $position === 'outer' ? '4px 0px' : 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const FacetAccordionSummary = styled(AccordionSummary)(({ $position }: { $position: 'inner' | 'outer' }) => ({
padding: $position === 'outer' ? '4px 0px' : 0,
const FacetAccordionSummary = styled(AccordionSummary)(({ $position, theme }: { $position: 'inner' | 'outer' }) => ({
padding: $position === 'outer' ? theme.spacing(0.5, 0) : 0,

Comment on lines +68 to +70
const FormLabelText = styled(Typography)({
marginRight: '2px',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const FormLabelText = styled(Typography)({
marginRight: '2px',
});
const FormLabelText = styled(Typography)(({ theme }) => ({
marginRight: theme.spacing(0.25),
}));

Comment on lines +21 to +22
const interPadding = `${16 * 0.6}px`;
const sidePadding = '64px';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use rem for these?

Suggested change
const interPadding = `${16 * 0.6}px`;
const sidePadding = '64px';
const interPadding = '.6rem';
const sidePadding = '4rem';

Comment on lines +72 to +83
const sharedArrowStyles = {
verticalAlign: 'text-top',
fontSize: '1.1rem',
};

const ArrowUpOn = styled(ArrowUpward)({
...sharedArrowStyles,
});

const ArrowDownOn = styled(ArrowDownward)({
...sharedArrowStyles,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should work:

Suggested change
const sharedArrowStyles = {
verticalAlign: 'text-top',
fontSize: '1.1rem',
};
const ArrowUpOn = styled(ArrowUpward)({
...sharedArrowStyles,
});
const ArrowDownOn = styled(ArrowDownward)({
...sharedArrowStyles,
});
const sharedArrowStyles = {
verticalAlign: 'text-top',
fontSize: '1.1rem',
} as const;
const ArrowUpOn = styled(ArrowUpward)(sharedArrowStyles);
const ArrowDownOn = styled(ArrowDownward)(sharedArrowStyles);

as const indicates that the properties of sharedArrowStyles are guaranteed not to change, so TypeScript narrows their values to text-top and 1.1rem respectively as opposed to string and string, which might be too loosely typed for styled-components' liking.

import { Entity } from 'js/components/types';

const donorMetadataPath = 'mapped_metadata';
const sampleMetdataPath = 'metadata';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const sampleMetdataPath = 'metadata';
const sampleMetadataPath = 'metadata';

},
dataset: {
donor: `donor.${donorMetadataPath}`,
sample: `source_samples.${sampleMetdataPath}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sample: `source_samples.${sampleMetdataPath}`,
sample: `source_samples.${sampleMetadataPath}`,

context/app/static/js/components/search/Search.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,45 @@
import React, { useCallback, useState } from 'react';
import { trackSiteSearch, trackEvent } from 'js/helpers/trackers';
import SearchBarComponent from 'js/shared-styles/inputs/SearchBar';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also potentially outside the scope of this PR, but maybe we can add a submit button to this component? It wouldn't hurt to have an e.g. ArrowRight icon at the end of the text input to indicate the form action's presence

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this mapping live in the repo? If so, do we have a mechanism to update it to reflect Zhou's changes?

context/package.json Outdated Show resolved Hide resolved
@john-conroy
Copy link
Collaborator Author

The publications/collections issues are just because I'm behind main here. Should be fixed with a merge.

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.

2 participants