-
Notifications
You must be signed in to change notification settings - Fork 36
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
[DOP-1219] Searchbar Part 1: Desktop Searchbar UI #223
Conversation
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.
This looks good! Are you able to generate a staging link? Should be doable just by running make stage
in the snooty root after building.
With regard to CSS, this is pretty similar to the direction we're heading towards. You can check out the components/landing/
directory for reference, as these are some newer components we've worked on. For that work, we started using a theme (theme/docsTheme.js
) that is nearly the same as UP's theme, so feel free to use that as well. I'd encourage you to add any scaffolding you think would be helpful!
Also, I think adding unit tests for these components would be helpful. No worries if you were planning to to add that later on!
package.json
Outdated
@@ -67,6 +67,7 @@ | |||
"@leafygreen-ui/leafygreen-provider": "^1.0.0", | |||
"@leafygreen-ui/modal": "^3.0.1", | |||
"@leafygreen-ui/palette": "^2.0.0", | |||
"@leafygreen-ui/text-input": "^2.0.0", |
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.
Version 3.0.0 of this package was just released, so let's use that!
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.
Oh wow literally today haha
src/components/Navbar.js
Outdated
className="navbar" | ||
data-navprops={this.navprops} | ||
style={{ position: 'absolute' }} | ||
></div> |
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.
[nit] Any reason why this wouldn't just be a <div />
?
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.
Nope, I was playing around with the implementation using children but never switched back.
import Icon from '@leafygreen-ui/icon'; | ||
import SearchDropdown from './SearchDropdown'; | ||
|
||
const GO_BUTTON_COLOR = '#E4F4F4'; |
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.
Looks like this is green.light3
in the LeafyGreen palette package! (source)
I think for unit testing I was hoping to add to the existing I'll also generate a staging link once these changes are made and update the description accordingly, thanks! |
Got the staging link to work! I didn't update docs-tools for it but will work on locally doing so to remove the existing searchbar from the nav. Not a real issue for now though |
thanks! |
Staging Link
(The target branch is the feature branch for the project, also the addition to package/lock.json has been approved)
This PR adds in a basic searchbar (not mobile-responsive, desktop only) which uses LeafyGreen components. Locally I disabled the navbar in docs-tools, but this will show up even without doing so.
I updated the
tabindex
but also noticed this is a bit odd on Safari so will continue to look into thatCurious how the CSS style is compared to what the team traditionally does, so feedback there would be helpful!