-
Notifications
You must be signed in to change notification settings - Fork 846
Instant Search: formatting improvements for mobile #14022
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
Instant Search: formatting improvements for mobile #14022
Conversation
| onClick={ this.onClick } | ||
| /> | ||
| </h3> | ||
| <div className="jetpack-instant-search__search-result-minimal-header"> |
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.
Just adding a wrapper element here.
gibrown
left a comment
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.
I feel like we should move the date between the title and the content? It feels odd to me floating above. Or maybe it just needs to be smaller. I guess another option is we could show the url rather than the date. When the date matters, it tends to be in the URL, and there are a lot of sites where date doesn't matter.
I think this also breaks margins on some other themes. 2015: http://gibrown.wpsandbox.me/demo/?s=related%20psts&blog_id=20115252&theme=twentyfifteen
I use this on my site to quickly test other themes:
function filter_theme( $theme ) {
if ( $_GET['theme'] ) {
$theme = sanitize_key( $_GET['theme'] );
}
return $theme;
}
add_filter( 'stylesheet', 'filter_theme', 1 );
add_filter( 'template', 'filter_theme', 1 );
cc @keoshi
I agree. Moving the date, which is accessory information, between the title and content would bring more attention to the relevant info (matched terms in title/content). To the best of my knowledge, the date is never part of a search result, correct? Google also does an interesting thing with dates, by including them in the content's block: I think that's overdoing it for our context, but cool to keep the blocks tidy nonetheless. As for the URL, I don't think it's relevant if we show the date by itself, since you're already searching in a single domain (I'm assuming?) and it would just add a lot of |
Correct, it should never be a part of the matching content.
So currently everything is for a single domain, but cross site search does work for VIPs in the existing Jetpack Search, and would be easy for us to implement and I could see us doing it early next year. It is probably a week of work. When a result has no content, then we do use the path as a fallback: The other fallback we have is tags and categories: So I think there could be something where we always show the path in these results, and then also show the domain when there are multiple sites. |
|
That's great info to know, @gibrown — thank you! |
|
Some good feedback here about date placement (or substituting it with other things). Let's continue discussing that and I'll close out this PR for now. I've created two PRs with changes from here that are ready to go:
|



Address feedback from @joanrho on the mobile experience when using the standard 'minimal' result format.
https://cloudup.com/cK8mBUIKy95/f
Changes proposed in this Pull Request:
On the smallest breakpoint:
On larger breakpoints:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
instant-search-master.Testing instructions:
Follow the setup instructions in https://github.com/Automattic/jetpack/blob/instant-search-master/modules/search/instant-search/README.md.
Check the results at different breakpoints. Example query:
/?s=card&blog_id=84860689Proposed changelog entry for your changes: