Skip to content
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

add learning materials tab #1132

Merged
merged 2 commits into from
Jun 21, 2024
Merged

add learning materials tab #1132

merged 2 commits into from
Jun 21, 2024

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Jun 20, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4620

Description (What does it do?)

This PR adds a "Learning Materials" tab to the search page and channel pages

Screenshots (if ap

Screenshot 2024-06-20 at 4 47 03 PM propriate): Screenshot 2024-06-20 at 4 35 03 PM Screenshot 2024-06-20 at 4 35 18 PM Screenshot 2024-06-20 at 4 35 37 PM

How can this be tested?

On this branch:

  1. View the search page locally.
  2. You should see 4 tabs: "All", "Courses", "Programs", "Learning Materials".
    • Check that each tab shows the appropriate things
    • "Learning Materials" should show everything except courses and programs
  3. Check that tab counts do not change when switching tabs

* Because of this, we pass the setSearchParams function from the parent
* rather than from a new "instance" of `useSearchParams`.
*/
setSearchParams: UseResourceSearchParamsProps["setSearchParams"]
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 opened an issue about this in course-search-utils mitodl/course-search-utils#113 with more details.

@Ferdi
Copy link

Ferdi commented Jun 21, 2024

@ChristopherChudzicki in the main nav menu we have "learning materials" as tbd. Are you planning to remove tbd and link to this tab?

@ChristopherChudzicki
Copy link
Contributor Author

@ChristopherChudzicki in the main nav menu we have "learning materials" as tbd. Are you planning to remove tbd and link to this tab?

Good call. I'll add that.

}) => {
const [searchParams] = useSearchParams()
const activeTab =
TABS.find((t) => t.name === searchParams.get("tab")) ??
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously the active tab was based on resource_type query parameter.

Now the tabs are:

  • All tab="all" ... resource_type=[]
  • Courses tab="courses" ... resource_type=["course"]
  • Programs tab="programs" ... resource_type=["program"]
  • Learning Materials tab="learning-materials" ... resource_type=["video", "podcast", "podcast_episode", "video_playlist", "learning_path"].

I was a little torn about whether to infer the tab based on resource_type query param, or use a separate tab param (which is what I ended up doing).

Using a separate tab really simplifies URLs like href: querifiedSearchUrl({ tab: "learning-materials" }), in the navbar.

My thought is that in the future when we add a "resource_type" sidebar facet (for the LM tab), the URLs would look like ?tab=learning-materials&resource_type="video".

@sovsey sovsey requested a review from abeglova June 21, 2024 13:34
Copy link
Contributor

@abeglova abeglova left a comment

Choose a reason for hiding this comment

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

👍 Works great!

@ChristopherChudzicki ChristopherChudzicki merged commit 206823b into main Jun 21, 2024
12 checks passed
@odlbot odlbot mentioned this pull request Jun 21, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants