Conversation
shashankbrgowda changed the title 
...app/species-selector/views/species-selector-search-view/SpeciesSelectorSearchView.module.css Outdated Show resolved Hide resolved
...ontent/app/species-selector/views/species-selector-search-view/SpeciesSelectorSearchView.tsx Outdated Show resolved Hide resolved
src/content/app/genome-browser/components/interstitial/BrowserInterstitial.tsx Outdated Show resolved Hide resolved
...ontent/app/species-selector/views/species-selector-search-view/SpeciesSelectorSearchView.tsx Outdated Show resolved Hide resolved
...ontent/app/species-selector/views/species-selector-search-view/SpeciesSelectorSearchView.tsx Outdated Show resolved Hide resolved
| 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.
src/shared/components/interstitial-search/InterstitialSearch.module.css Outdated Show resolved Hide resolved
| 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
src/shared/components/interstitial-search/InterstitialSearch.tsx Outdated Show resolved Hide resolved
src/shared/components/layout/sidebar-modal/SidebarModal.module.css Outdated Show resolved Hide resolved
| 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 resultsContent = getResultsContent(searchMode); | ||
| | ||
| return ( | ||
| <div key={searchMode} className={styles.section}> |
There was a problem hiding this comment.
CollapsibleSection takes an optional className property. Given that, is there a need for this wrapper div?
| ); | ||
| }; | ||
| | ||
| const PageDetails = (props: { |
There was a problem hiding this comment.
There is a lot of layout shift during pagination. I wonder if there is anythig we can do to improve this.
jumps.mp4
We could at least apply the scrollbar-gutter: stable; CSS rule to always reserve space for the scrollbar.
| | ||
| 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
...app/species-selector/views/species-selector-search-view/SpeciesSelectorSearchView.module.css Outdated Show resolved Hide resolved
| 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 |
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