Update search UI in right hand panel#1276
Conversation
shashankbrgowda
changed the title
| const { featureSearchMode, searchResults, speciesList } = props; | ||
|
|
||
| return ( | ||
| <div className={styles.resultsWrapper}> |
There was a problem hiding this comment.
FeatureSearchResults component has a top-level div in it as well. I wonder if we give that component an optional className property, and pass whichever styles the resultsWrapper has via that property, would it remove the need for this wrapper div?
On the other hand, I see you also have VariantFeatureSearchResults, which uses the resultsWrapper; so maybe there is no need to change anything here.
| overflow-y: auto; | ||
| padding-left: 20px; | ||
| margin-top: 20px; | ||
| } |
There was a problem hiding this comment.
It's used in structural variants FeatureSearchModal.tsx
There was a problem hiding this comment.
Mmm, that's not good. FeatureSearchModal is something that is shown in the sidebar; it should not depend on the interstitial. Hmm 🤔
There was a problem hiding this comment.
I've pushed a commit to move the styles imported by structural variants FeatureSearchModal into its own styles file.
| expect(interstitialSearch).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('renders with interstitial mode', () => { |
There was a problem hiding this comment.
this test seems to be testing the same as the previous one
| overflow: auto; | ||
| padding: 21px calc(18px - var(--scrollbar-width)) 90px 20px; | ||
|
|
||
| padding: 21px calc(18px - var(--scrollbar-width)) 21px 20px; |
There was a problem hiding this comment.
Hmm, that 90px from the bottom was the same as --global-padding-bottom CSS variable, which we normally use to leave space in the bottom of various scrollable containers. Are we saying that in all sidebar modals we are going to have only 21px of space in the bottom?
There was a problem hiding this comment.
Yes, 90px padding was too much for search modal. 21px padding shouldn't cause any issues for other modals.
| } | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
I think this getPageDetails function, as well as the getResultsContent function below it should either be turned into components (like SidebarHelpSection or PageDetails below, or at the very least wrapped in a useMemo. As it is, these functions return completely new components every render cycle.
Ideally, changing text in an input box shouldn't cause everything around it to rerender. This is something that the compiler is promised to take care of; but we don't have the compiler yet.
rerendering.mp4
|
|
||
| const searchMatchAnchorClass = classNames( | ||
| styles.searchMatchAnchor, | ||
| styles[`searchMatchAnchor${mode}`] |
There was a problem hiding this comment.
searchMatchAnchor${mode} won't match the CSS class in SearchMatch.module.css, because mode is in lowercase (sidebar/interstitial) while the class name is in camelCase (searchMatchAnchorInterstitial). Same in VariantSearchMatches.tsx
|
Oof! This is not good: Screen.Recording.2026-03-16.at.13.15.26.mov |
| --image-button-default-svg-color: var(--color-blue); | ||
| --image-button-disabled-svg-color: var(--color-grey); | ||
| width: var(--browser-nav-button-size, 22px); | ||
| height: var(--browser-nav-button-size, 22px); |
There was a problem hiding this comment.
- Why does the variable have the word
browserin its name? - These are styles for a component (
SidebarSearch) that combines a lot of other components. Why does it need to expose variables for configuring nav buttons sizes at all?
| export type FeatureSearchAppName = | ||
| | 'speciesHome' | ||
| | 'genomeBrowser' | ||
| | 'entityViewer'; |
There was a problem hiding this comment.
I am of two minds about this.
On the one hand, we have a central place for listing our apps. On the other hand, that place uses an enum, and my hope is that we gradually get rid of enums. On the one hand, it seems like a good idea to not proliferate places that list apps. On the other hand, I don't know if listing possible apps specifically for feature search component has its benefits.
There was a problem hiding this comment.
The AppName used is from here - https://github.com/Ensembl/ensembl-client/blob/main/src/shared/state/in-app-search/inAppSearchSlice.ts#L19
So this is existing code moved to a common place
|
I still don't quite understand what the rules for search state persistence are, and whether they aren't too complicated to us to push back against. For example, here, with the preservation of rs699 even after we explicitly close the search sidebar. I would personally so much prefer that we could just clear the search state every time we unmount the search sidebar component; but I don't know if I can convince others to agree to that. search-persistence.mp4 |
|
|
||
| const featureSearchQueries = useAppSelector((state) => | ||
| getFeatureSearchQueries(state, app, genomeId) | ||
| ); |
There was a problem hiding this comment.
So now that we no longer persist query information when we move between apps, or between different genomes, why are we still storing search queries in redux? Why aren't they in local state?
There was a problem hiding this comment.
From the web-standup slack chat, Its applicable only for Sidebar search.
Right hand panel search should not hold state for any element (gene, variation, transcript)
There was a problem hiding this comment.
Are we keeping the redux state just for the interstitial search?
| type StateForApp = Record<string, { queries: Queries }>; | ||
|
|
||
| type State = Record<AppName, StateForApp>; | ||
| // app -> genomeId -> queries mapping |
There was a problem hiding this comment.
Is this comment still correct? I see app name is gone from the file
There was a problem hiding this comment.
yes. Its now FeatureSearchAppName
azangru
left a comment
azangru
left a comment
There was a problem hiding this comment.
I was hoping the relaxed requirements for state management would allow us to get rid of the redux state responsible for feature search; but even if not, it's not a blocker. Leaving it up to you to decide if you want to keep it.
We can keep it for now as its existing functionality in prod and the state is simple like before. |
|
Thanks for the review @azangru |
3 participants
Description
This pr updates the search UI in the right hand panel of SS, GB and EV.
Related JIRA Issue(s)
https://embl.atlassian.net/browse/ENSWBSITES-2968
Deployment URL(s)
http://side-bar-search-panel.review.ensembl.org