-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(search): more search improvements #18705
Conversation
Size Change: -14.9 kB (-1%) Total Size: 1.99 MB
|
4b64e1c
to
f1d60f2
Compare
f1d60f2
to
8d3666a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love search now, the results seem pretty good.
As you may notice, I pushed a small change to make skeleton sizing joy-sparking. One visual bug I noticed: in one spot we're using Just pushed a fix.bg-bg-3000-light
, which seems pretty wrong, because that's the light mode version being hard-coded. Should be bg-bg-3000
.
The whole command bar is also in a dire need of hover/focus states, it's pretty hard to use otherwise. So I'm gonna give the green check now, but we that's one improvement we've got to make before rolling this out.
@@ -12,7 +12,7 @@ const globals = { | |||
} | |||
|
|||
module.exports = { | |||
ignorePatterns: ['node_modules', 'plugin-server'], | |||
ignorePatterns: ['node_modules', 'plugin-server', 'cypress'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add this as ESLint was complaining about issues due to our Cypress code not being properly typed
@@ -39,7 +39,9 @@ export function CommandBar(): JSX.Element | null { | |||
return ( | |||
<CommandBarOverlay> | |||
<div | |||
className="w-full h-160 max-w-lg bg-bg-3000 rounded overflow-hidden flex flex-col border shadow" | |||
className={`w-full ${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shadow
amounts to a border-bottom in 3000 mode, which makes sense for buttons, but does not look great here IMO. Are we getting rid of the modal shadow in 3000 completely @corywatilo?
59969fd
to
b12c93f
Compare
Problem
The search had some problems... see below.
Changes
This PR:
How did you test this code?
Added tests & clicked around