Skip to content

Conversation

@outdoor2kode
Copy link
Contributor

@outdoor2kode outdoor2kode commented Jul 30, 2024

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

Big text Text under the search bar
image image

Testing

  1. Search on the homepage.
  2. Play around with various filters.
  3. Search on the Course / Lesson / Learning pathway archive page, and the results should be on the same page (ie, the header, hero section as well as the filters should still be there)

Screencast

search.mov

@outdoor2kode outdoor2kode requested a review from ryelle July 30, 2024 19:54
@outdoor2kode outdoor2kode self-assigned this Jul 30, 2024
@outdoor2kode outdoor2kode added [Type] Enhancement New feature request for the Learn website. [Component] Learn Theme Website development issues related to the Learn theme. labels Jul 30, 2024
@ndiego
Copy link
Member

ndiego commented Jul 30, 2024

Thanks @renintw. Regarding the placement of the Search on the Homepage, can we mirror the placement on Forums exactly? This will provide consistency. In the video, it looks like there is a bit of extra padding/margin at the top.

image

@outdoor2kode outdoor2kode added this to the Learning Pathways launch milestone Jul 30, 2024
@outdoor2kode
Copy link
Contributor Author

can we mirror the placement on Forums exactly

Oh, no problem, I was referring to the Showcase, will change that. And it appears that Plugins also needs an update.

@ndiego
Copy link
Member

ndiego commented Jul 30, 2024

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.

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.

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?

@kathrynwp
Copy link
Collaborator

I like "Search for resources", what do you think @kathrynwp?

Agreed, I like it!

@kathrynwp
Copy link
Collaborator

What if we went with "Search Learn WordPress" as a static title?

I like this for a static title, too.

@ndiego
Copy link
Member

ndiego commented Jul 30, 2024

The only thing I am unsure about is whether we need some sort of label distinguishing between courses and lessons. Visually, I think it's relatively clear which one is a course, but less clear that a lesson is a lesson based on the visuals, if that makes sense. I personally would be fine leaving it the way it is, but I could easily be swayed in the other direction. 😅

Course Lesson
image image

@kathrynwp
Copy link
Collaborator

@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.

@outdoor2kode
Copy link
Contributor Author

outdoor2kode commented Jul 30, 2024

can we mirror the placement on Forums exactly

39b64fa

Learn Forum
Screenshot 2024-07-31 at 05 33 36 Screenshot 2024-07-31 at 05 34 02

What if we went with "Search Learn WordPress" as a static title?

I like this for a static title, too.

3a60523

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.

Version A Version B Course Archive
Screenshot 2024-07-31 at 05 38 01 Screenshot 2024-07-31 at 05 36 59 image

@ndiego
Copy link
Member

ndiego commented Jul 30, 2024

Version A or B? Version A has the same gap as the homepage. Version B has the same gap as the course archive.

The homepage looks good, and I would go with Version B.

Copy link
Contributor

@ryelle ryelle left a 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} /-->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the content type dropdown on Lesson & Course archives?

Screenshot 2024-07-30 at 5 13 19 PM

Copy link
Contributor Author

@outdoor2kode outdoor2kode Jul 30, 2024

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.

Copy link
Contributor

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)

Copy link
Contributor Author

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

Copy link
Member

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.

@outdoor2kode outdoor2kode force-pushed the 2744-add-search-bar branch from 12a21f1 to 33e992c Compare July 31, 2024 13:22
@kathrynwp
Copy link
Collaborator

Do we really need the content type dropdown on Lesson & Course archives?

I think this could be a useful toggle to have, although not strictly necessary.

@outdoor2kode outdoor2kode force-pushed the 2744-add-search-bar branch from d1f703d to 040de3e Compare July 31, 2024 14:54
Copy link
Contributor

@ryelle ryelle left a 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 );
Copy link
Contributor

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).

Suggested change
$label = get_label_by_slug( $selected_slug, $options );
$label = $options[ $selected_slug ] ?? $options['any'];

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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;
Copy link
Contributor

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.

Suggested change
return home_url() . $filtered_path;
return home_url( $filtered_path );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 999b6c2

Comment on lines 37 to 42
$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'] : '';
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +18 to +19
* 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.
Copy link
Contributor

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 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, thanks!

Copy link
Contributor

@ryelle ryelle left a 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 👍🏻

@outdoor2kode outdoor2kode merged commit 5bec934 into trunk Jul 31, 2024
@outdoor2kode outdoor2kode deleted the 2744-add-search-bar branch July 31, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Component] Learn Theme Website development issues related to the Learn theme. [Type] Enhancement New feature request for the Learn website.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Add Search box to Learn homepage

5 participants