-
Notifications
You must be signed in to change notification settings - Fork 122
Add Search bar to the homepage #2801
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
Conversation
Oh, no problem, I was referring to the Showcase, will change that. And it appears that Plugins also needs an update. |
Hmm, you are right. It looks like the search field on pages in the Learn category on WordPress.org is in a slightly different spot than those under Extend and Showcase. My recommendation is we update it so it's similar to Forums and then address post launch if we want to make everything consistent.
I like the text underneath the search field, but I do think the page needs a title. What if we went with "Search Learn WordPress" as a static title? |
Agreed, I like it! |
I like this for a static title, too. |
|
@ndiego Definitely see your point. Personally I think this should wait til after launch, as there are a few related issues that relate to distinguishing between:
...and they should probably all be dealt with holistically. |
Version A or B? Version A has the same gap as the homepage. Version B has the same gap as the course archive. The current implementation is version B.
|
The homepage looks good, and I would go with Version B. |
ryelle
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.
The search is working, but I had a few other comments about some details.
|
|
||
| <!-- wp:group {"style":{"spacing":{"blockGap":"0"}},"layout":{"type":"flex","flexWrap":"nowrap"},"className":"wporg-query-filters"} --> | ||
| <div class="wp-block-group wporg-query-filters"> | ||
| <!-- wp:wporg/query-filter {"key":"content_type","multiple":false} /--> |
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.
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.
Good question. I added it because if selecting a course or lesson from the search all page, without this filter, you can't go back from the course or lesson archive. Seems to be somewhat inconsistent in the searching process starting from homepage.
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.
Ah, I see, if you do the search first and then filter by type, it moves over to the archive template for those search results.
IMO, people can use the back button to get back to all search results if they filter the post type— it's more confusing to me just landing on /lessons/ and seeing the content type dropdown, since it brings you to a totally different page (also with different filtering options) if you use it.
Open to other opinions though, cc @ndiego @kathrynwp (not sure who else has input here)
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 think both options are fine with me. What might ultimately influence my decision is which path the users use most frequently. If most users explore the site from the homepage search, then having a content type filter on the /lessons/ or /courses/ pages would provide a better experience IMO.
Alternatively, we could consider showing the filter on the /lessons/ or /courses/ pages only when users use the content type filter from the all search results page and are redirected to those pages. If they visit those pages directly, the filter would not be displayed.
I've removed the filter from the lesson and course archive for now. Let me know if we want it back.
b6be6ea
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.
IMO, people can use the back button to get back to all search results if they filter the post type— it's more confusing to me just landing on /lessons/ and seeing the content type dropdown, since it brings you to a totally different page (also with different filtering options) if you use it.
That's a good point. I have no strong opinions though. I found it nice that I could switch over to Courses from Lessons, but I'm not a typical user.
wp-content/themes/pub/wporg-learn-2024/patterns/search-all-content.php
Outdated
Show resolved
Hide resolved
12a21f1 to
33e992c
Compare
I think this could be a useful toggle to have, although not strictly necessary. |
d1f703d to
040de3e
Compare
ryelle
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.
Looking good! I still have a few minor code improvement suggestions around the filter code.
| ); | ||
|
|
||
| $selected_slug = $wp_query->get( 'post_type' ) ? $wp_query->get( 'post_type' ) : 'any'; | ||
| $label = get_label_by_slug( $selected_slug, $options ); |
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.
Since these are keyed by the slug, do you need an extra function? I think this should work for both here and get_student_course_options (though the fallback there should be all).
| $label = get_label_by_slug( $selected_slug, $options ); | |
| $label = $options[ $selected_slug ] ?? $options['any']; |
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.
yeah, you're exactly right, those logic are unnecessary..
I removed the ?? $options['any']; as $options[ $selected_slug ] will always have a value.
18c28a6
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.
It will, but it could be something not in the list — if someone manually types in a post type, for example. Adding the fallback works as a guard, just in case.
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.
ah, yes. Missed the fact that it fetches the value from the URL. ae09e8e
| $path = isset( $parsed_url['path'] ) ? $parsed_url['path'] : ''; | ||
| $filtered_path = preg_replace( '#/page/\d+/?$#', '', $path ); | ||
|
|
||
| return home_url() . $filtered_path; |
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.
You can pass the filtered_path to home_url as a parameter, it accepts a path parameter.
| return home_url() . $filtered_path; | |
| return home_url( $filtered_path ); |
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! 999b6c2
| $current_url = home_url( add_query_arg( array(), $wp->request ) ); | ||
|
|
||
| // Remove the page query var. | ||
| // If the path retains "page," the filtered result might be a 404 page. | ||
| $parsed_url = wp_parse_url( $current_url ); | ||
| $path = isset( $parsed_url['path'] ) ? $parsed_url['path'] : ''; |
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 believe $wp->request will be just the path, no query args, so I don't think you need to do this extra parsing.
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.
| * Since the search results for courses and lessons still use their respective archive templates, | ||
| * we need to update the header template part to display the correct title. |
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.
Documentation improvements like this could be committed right to trunk IMO, to avoid extra changes in PRs like this. No change requested for this PR, just a suggestion for the 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.
Noted, thanks!
ryelle
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.
Great, I think this is good to merge now 👍🏻









Resolves #2744
As the screencast shows, when searching on the homepage, it goes to the newly added template - search-front-page. When switching to another content type, "course" or "lesson", the behavior remains as is - stay in the archive-course, or archive-lesson templates, displaying their own dedicated UI and filters. And it switches back to the search-front-page template when the content type "Any" is chosen.
The unclear and confusing wording of the filter mentioned in #2787 will be addressed in another PR.
Please also feel free to provide feedback on the flow or design. For example like including a label somewhere on the card to allow users to know whether they are looking at a Course or a Lesson, or do we still want the big text on top of the search page when there's already a text functioning similarly under the search bar. @ndiego @kathrynwp @WordPress/meta-design
Testing
Screencast
search.mov