Skip to content

feat: clickable tag filters with shareable URL on /courses#558

Open
adrianbp2 wants to merge 4 commits into
OxfordRSE:mainfrom
adrianbp2:feat/clickable-tag-filters
Open

feat: clickable tag filters with shareable URL on /courses#558
adrianbp2 wants to merge 4 commits into
OxfordRSE:mainfrom
adrianbp2:feat/clickable-tag-filters

Conversation

@adrianbp2

Copy link
Copy Markdown

Closes #557

Changes

  • TagChip now accepts a linkToFilter prop — renders as a <Link> to /courses?tag=X
  • CourseCard and course detail page tags are now clickable, navigating to the filtered courses view
  • /courses page reads filter state (tag, level, q, lang) from URL query params on mount and on in-page navigation
  • Filter changes sync back to the URL via router.replace (shallow) — shareable links without polluting browser history

How to test

  1. Go to /courses — click any tag chip → URL updates and filter applies
  2. Go to /courses/[id] — click a tag → navigates to /courses?tag=X with filter active
  3. Copy a filtered URL and open in new tab → filter is pre-applied
  4. Use browser back/forward — no extra history entries from filter changes

- TagChip accepts linkToFilter prop to render as a link to /courses?tag=X
- Courses page reads filter state from URL query params on mount/navigation
- Filter changes sync to URL with router.replace (no history pollution)
- Tags on CourseCard and course detail page link to filtered courses view

@alasdairwilson alasdairwilson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, thank you for your contribution.

I have some generic feedback that would be good to sort out:

  1. We should avoid manually concatenating basePath for internal navigation here. next/link already knows how to handle the app base path, so this should just link to "/courses" rather than building by hand.
  2. We also should not manually construct the query string. It would be cleaner and safer to pass an object like href={{ pathname: "/courses", query: { tag } }} and let Next handle encoding/serialization. It is safer and clearer.
  3. When trying this out, there is a bit of ugliness due to the SSR page being unfiltered so you get layout changes when the page is re-rendered. If we want shareable filtered URLs to work cleanly, we should initialize the filters during SSR rather than only reading them in a client-side effect. That would probably mean reading the filter values from the request query in getServerSideProps and passing them in as initial state vai the props return, instead of always starting empty and then syncing from router.query in a useEffect.

@adrianbp2

Copy link
Copy Markdown
Author

Thx for feedback @alasdairwilson, here are the changes.

@alasdairwilson alasdairwilson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, thanks for making those changes.

At a minimum in terms of tests, we would need to test that:

  1. clicking on a tag takes you to /courses with the correct tag set
  2. url on adding/removing tag/levels etc

but there are some others that could be useful/ clear filters clearing the url, checking the initial render

I don't mind doing these if you don't want to/don't know how.

@alasdairwilson

Copy link
Copy Markdown
Member

I know you did exactly what I asked for in terms of replacing the getstaticprops with ssr...but having seen that code change I am wondering if it is the right thing...seems a shame to dump the cached page for this but also I did hate the layout change.
I am just pondering because this page was firmly on the side of the static common pages where all the index pages basically were serving ssg'd pages and this has now swapped courses to fully ssr. we could keep that by delaying the rendering of course cards till we had read the query.

Since I don't really know the best solution I'd leave what you have (with get server side props) for now, it can always be changed later.

@adrianbp2

Copy link
Copy Markdown
Author

I've added e2e tests covering all the cases you mentioned.

@alasdairwilson

Copy link
Copy Markdown
Member

Ok apologies for taking so long to get back to this,

This change comes with at least one bug:

The issue is that on /courses it is keeping its local filter state after a same-route navigation like /courses?tag=programming -> /courses?tag=tooling so if you navigate once to courses you now cant navigate again, even though Next is fetching new SSR props in the background. But this made me realise the tag chips on /courses shouldn’t be acting like fresh page navigation at all, they should just toggle the existing filters on the page

we should also nav from the same tagchips on the home page

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clickable course tags take you to filtered course page

2 participants