-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Marketplace: Add search header #62080
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~217 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
const SearchHeader = ( props ) => { | ||
const { doSearch, search, siteSlug, title, searchBoxRef } = props; | ||
|
||
return ( | ||
<div className="plugins-browser__searchbox-container"> | ||
<div className="plugins-browser__searchbox-header">{ title }</div> | ||
<div className="plugins-browser__searchbox" ref={ searchBoxRef }> | ||
<SearchBox doSearch={ doSearch } search={ search } /> | ||
</div> | ||
<PopularSearches | ||
siteSlug={ siteSlug } | ||
searchTerms={ [ 'shipping', 'seo', 'portfolio', 'chat', 'mailchimp' ] } | ||
/> | ||
</div> | ||
); | ||
}; | ||
|
||
const PopularSearches = ( props ) => { | ||
const { searchTerms, siteSlug } = props; | ||
const translate = useTranslate(); | ||
|
||
return ( | ||
<div className="plugins-browser__recommended-searches"> | ||
<div className="plugins-browser__recommended-searches-title"> | ||
{ translate( 'Most popular searches' ) } | ||
</div> | ||
|
||
<div className="plugins-browser__recommended-searches-list"> | ||
{ searchTerms.map( ( searchTerm ) => ( | ||
<a | ||
href={ `/plugins/${ siteSlug || '' }?s=${ searchTerm }` } | ||
className="plugins-browser__recommended-searches-list-item" | ||
> | ||
{ searchTerm } | ||
</a> | ||
) ) } | ||
</div> | ||
</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.
Can we move new components under plugins
for the SearchHeader
? This file has already become too big and hard to handle.
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.
Bundling their respective CSS and removing it from client/my-sites/plugins/plugins-browser/style.scss
too.
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.
sure, was thinking on doing that before pushing the PR to review
<ActionsContainer ref={ actionsRef }>{ actions }</ActionsContainer> | ||
{ children } |
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.
What's the rationale behind this change? This component was created to have two sections:
- left -
Breadcrumb
- right -
ActionsContainer
(as arbitrary children)
Do we really need a third section? Does unwrapping the children
create any regressions?
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.
@cpapazoglou I did this to be able to accommodate the sticky behavior of the SearchBox
component, right now the FixedNavigationHeader
behavior is to render the breadcrumbs and conditionally rendering the action section which is the one where the children's are rendered an example of this behavior can be seen on the woocommerce-installation
flow.
To be able to stick the search bar without removing the actions at the right I added this changes, but I wonder if there is a better approach which doesn't impact the FixedNavigationHeader
.
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.
Also I tested the flow where we use the FixedNavigationHeader
and no regressions or weird behavior where caused.
8b17774
to
4cbbeaa
Compare
f8af9a9
to
0325a0b
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.
LGTM
</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.
Thank you for breaking things down like this. Everything being simple top level components is a nice way to make things re-usable in future.
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.
Thanks for addressing all feedback and also splitting this which makes it far easier to review and merge! LGTM!
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7205638 Thank you @Rolilink for including a screenshot in the description! This is really helpful for our translators. |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
Screenshots
Testing instructions
http://calypso.localhost:3000/plugins
.seo
on the search bar it should showseo
related plugins.portfolio
recommended search, it should showportfolio
related plugins.http://calypso.localhost:3000/plugins/:siteSlug
.seo
on the search bar it should showseo
related plugins.portfolio
recommended search, it should showportfolio
related plugins.Related to #61863