Search Results: new styling#59834
Conversation
| @@ -1,5 +1,5 @@ | |||
| .button { | |||
| padding: 0.5rem; | |||
| padding: 0 0.5rem; | |||
There was a problem hiding this comment.
Changed the copy path button to only have horizontal padding so it is not forcing the padding for the whole header bar.
| <div className={classNames(styles.searchResultMatch, 'p-2')}> | ||
| <small>{result.pathMatches ? 'Path match' : 'File contains matching content'}</small> | ||
| </div> | ||
| </ResultContainer> |
There was a problem hiding this comment.
Removed the currently-mostly-useless card for file matches
| .result-container { | ||
| &:last-child { | ||
| border-bottom-width: 1px; | ||
| } | ||
|
|
||
| &:not(:last-of-type) { | ||
| // Prevents the sticky items below from affecting | ||
| // anything outside of the result container. | ||
| isolation: isolate; | ||
| margin-bottom: 1rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved the borders off the result container so they still make sense even when we do not have any contents for the results (like for path matches).
I'm honestly not sure what the isolate was for, but AFAICT it isn't doing anything now.
| // With 0 value there is a rendering bug in Safari where this block | ||
| // doesn't fit tight enough and hence it's leaving a gap between sticky | ||
| // header and top of the scrolling block | ||
| top: -1px; |
There was a problem hiding this comment.
Tested on Safari and this is no longer needed. And it was causing the header to shift on scroll, which made it feel a tiny bit janky.
| // TODO(taiyab): this needs to be fit into the design system | ||
| :global(.theme-light) & { | ||
| background-color: #f4f7f9; | ||
| } | ||
| :global(.theme-dark) & { | ||
| background-color: #22283b; | ||
| } |
There was a problem hiding this comment.
There weren't good colors in our existing variables, so we hard-coded colors here. @taiyab is to follow up and update our color palette
| .result-container { | ||
| &:last-child { | ||
| border-bottom-width: 1px; | ||
| } | ||
|
|
||
| &:not(:last-of-type) { | ||
| // Prevents the sticky items below from affecting | ||
| // anything outside of the result container. | ||
| isolation: isolate; | ||
| margin-bottom: 1rem; | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of putting a border around the containers as a whole, we put a border on each inner element because for path results, now we don't have any children elements so we'd be doubling the header border.
| --secondary-2: #242936; | ||
| --secondary-3: #1f232e; | ||
| --secondary-4: #545967; | ||
| --secondary-4: #1e222f; |
There was a problem hiding this comment.
This color was listed incorrectly, so we updated it from the designs.
| --modal-bg: #{rgba($gray-04, 0.5)}; | ||
| --sidebar-bg: var(--color-bg-1); | ||
| --sidebar-border-color: var(--border-color-2); | ||
| --collapse-results-color: var(--gray-08); |
There was a problem hiding this comment.
No reason for this to be a specific variable, especially a global.
| position: relative; | ||
| display: flex; | ||
| flex-direction: column; | ||
| background-color: var(--body-bg) !important; |
There was a problem hiding this comment.
yay, one less !important!
| &:hover { | ||
| border-color: var(--border-color-2); | ||
| } |
There was a problem hiding this comment.
@taiyab the border color change on hover no longer made sense to me since our results are much flatter now, so I removed it. Do we want search results to still react on hover somehow?
| position: relative; | ||
| display: flex; | ||
| flex-direction: column; | ||
| background-color: var(--body-bg) !important; |
vovakulikov
left a comment
There was a problem hiding this comment.
First of all thanks @camdencheek, it looks like a small changes in code but it feels very good, and I think it matches with other panels better too (like file preview and filters panel)
Not a blocker but one thing that I've noticed is that we probably want to simplify repository card when we have just private badge without description, so this card won't take two rows cc @taiyab
There was a problem hiding this comment.
FIY this is what in my opinion we should do, ideally we should not have inline classes, even small spacing classes aren't good, thanks!
Why? Because it multiple sources of styles hence
- It might be hard to debug since there is no good way to override inline classes in the browsers freely
- Even this set of spacings makes spacing inconsistent enough (some use 0.25rem some use just spaces instead)
Ideally we should rely on properties like gap in the layout.
0db4316 to
2f0a97f
Compare
| // TODO(taiyab): this needs to be fit into the design system | ||
| :global(.theme-light) & { | ||
| background-color: #f4f7f9; | ||
| background-color: #f5f8fa; |
There was a problem hiding this comment.
FYI @taiyab, CI was complaining because the background color was just barely below the contrast threshold (4.48 vs 4.5), so I bumped this just a tiny bit lighter.
This updates the styling of the search results to match the designs. As it turns out, there wasn't a ton to do other than styling and spacing tweaks that we needed to do anyways.
A video of scroll behavior. Notice that the first header does not move when scrolled.
CleanShot.2024-01-24.at.16.51.08.mp4
@taiyab The header color for dark mode looks nice, but does not contrast well with the border color:
Test plan
Lots and lots of manual visual testing and tweaking. There should be no changes in behavior, only in style. I'll be doing an audit of the other places we display search results as well to make sure nothing surprising broke there before merge, but right now it's dinner time.