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

Feat: Add page number and searchTerm to url param #293

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lilyMrt
Copy link
Contributor

@lilyMrt lilyMrt commented May 21, 2024

Store search and page number in URL so links to specific searches can be shared.

Could be done without react-router-dom if new packages aren't wanted, this was just the most efficient way to do it.

Updates the SearchBar input placeholder to the search term from the query, I also considered changing the actual value of the input so the user can also edit the search from a URL without retyping the whole text but it would require adding state and changing how the submission works. Not too hard, just wasn't sure if it was worth it.

I think adding filters to URL might be a bit overkill?

Happy to hear any feedback or thoughts :)

@noman-land
Copy link
Owner

noman-land commented May 30, 2024

Thanks for this, @lilyMrt! There's a bit of crossover between what you've done here and my initial explorations with adding in a router here.

I want for users to be able to link to specific episodes, and eventually to specific bits of transcript, but also potentially to search results. So there's some unanswered UX questions around what it would look like for both an episode URL as well as search params to be present at the same time.

So what happens here? https://transcript.fish/episodes/143?search=digby.

Right now search results return the whole episode, which I'd like to change. I think maybe it would be nicer if search results only returned the subset of the transcript that contains the search text, which might appear multiple times throughout the episode.

So the way the search results are displayed now, there's no way to display both a single episode and the "search results" from just that one episode.

@noman-land noman-land added enhancement New feature or request typescript Updates to Typescript code ux Updates to user experience labels May 30, 2024
@noman-land noman-land added this to the Public beta milestone May 30, 2024
const [searchTerm, setSearchTerm] = useState(
searchParams.get('search') || ''
);
const [page, setPage] = useState(parseInt(searchParams.get('page') ?? '0'));
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not convinced having page number in the URL is a good idea. The tricky thing is that:

  1. It affects browser history so changing pages 10 times adds 10 new items to browser history and would mean when you pressed "back" in the browser, it would simply go back a page in the search results. This might be desirable but I'm not sure! This can also be disabled with a history.replace but...
  2. Page numbers tend to stick around in the URL after you no longer want them. So for example you bookmark transcript.fish but you're not super tech savvy so you bookmark page 2 by accident. Or share page 2 by accident and that leads to a kind of confusing experience.
  3. Sharing links to a specific page of a specific search seems of limited use. That search will potentially produce a different result in a week so you're basically guaranteed for that page to have different results than the ones you meant to link to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request typescript Updates to Typescript code ux Updates to user experience
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants