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 optional filtering of help posts #140

Merged
merged 6 commits into from
Jan 7, 2025
Merged

Add optional filtering of help posts #140

merged 6 commits into from
Jan 7, 2025

Conversation

abr-egn
Copy link
Contributor

@abr-egn abr-egn commented Dec 26, 2024

Fixes #138 :)

My Go and Javascript are both very out of practice, so apologies on that front!

This adds simple server-side filtering of posts to the "Help and Information" forum, selected by locale. The RU and JP locales didn't seem to have a direct equivalent for that forum so I've left those unset, which means clicking on the toggle will be a no-op for those.

This will incur more db fetches per page load, unfortunately. That might be avoidable in the DynamoDB backend if more metadata is stored along with the gzipped blob so the query can directly filter on properties like forum ID, but that would be a much more invasive change and didn't seem proportionate to the feature scope.

I added the toggle link opposite the "Next Page" link at page bottom but UX is absolutely not my strong suit so I don't know if that's actually a good place for it :)

@abr-egn abr-egn marked this pull request as ready for review December 26, 2024 02:51
@ccbrown ccbrown self-requested a review December 29, 2024 05:42
@ccbrown
Copy link
Owner

ccbrown commented Dec 29, 2024

Thanks for the PR! I'm a bit slammed right now due to the holiday season, but I'll take a look as soon as I'm able.

@ccbrown
Copy link
Owner

ccbrown commented Jan 3, 2025

Okay, sorry for the delay here, but I finally have some time to look at this now. Functionally, everything looks great! The backend code looks good, and the filter does seem to do a good job:

Screenshot 2025-01-02 at 22 11 12

I would like to do better with the frontend though. Instead of a subtle link at the bottom of the list, I propose:

  • Move it to the top of the list, just to the left of the RSS button.
  • Don't keep the state in the page URL. Instead, I think it would be better for users if we stored the state in local session data. That way their preference will persist any time they visit.
  • After clicking the link, there's a delay before the filtered data is loaded. Users should have a clear indicator that something is happening. The easiest thing to do is probably just clear the table. And maybe also add a "Loading..." row to the table.

What do you think?

@abr-egn
Copy link
Contributor Author

abr-egn commented Jan 3, 2025

I would like to do better with the frontend though. Instead of a subtle link at the bottom of the list, I propose:

  • Move it to the top of the list, just to the left of the RSS button.

  • After clicking the link, there's a delay before the filtered data is loaded. Users should have a clear indicator that something is happening. The easiest thing to do is probably just clear the table. And maybe also add a "Loading..." row to the table.

Sounds good, I'll update the PR with those this weekend :)

  • Don't keep the state in the page URL. Instead, I think it would be better for users if we stored the state in local session data. That way their preference will persist any time they visit.

I'm not sure I agree with this one - it doesn't seem great that a link with the same URL would render a potentially completely different list of posts depending on whether the person clicking the link had that bit set locally or not.

@ccbrown
Copy link
Owner

ccbrown commented Jan 3, 2025

I'm not sure I agree with this one - it doesn't seem great that a link with the same URL would render a potentially completely different list of posts depending on whether the person clicking the link had that bit set locally or not.

Maybe it would be better to use a checkbox instead of a link then?

@abr-egn
Copy link
Contributor Author

abr-egn commented Jan 4, 2025

I'm not sure I agree with this one - it doesn't seem great that a link with the same URL would render a potentially completely different list of posts depending on whether the person clicking the link had that bit set locally or not.

Maybe it would be better to use a checkbox instead of a link then?

Oh, sorry, I think I explained this badly. Say user A is looking at page two of the tracker with filtering on, and they see a funny sequence of post titles, so they copy the URL and send it to user B. With the filtering as part of local session data, user B will see a very different list of posts, get confused, and file a bug report you'll have to deal with :P

Whether the UI element to do the toggle is a link or a checkbox doesn't matter much for this concern, it's just whether a given URL will render the same set of posts when transported around to different contexts.

@ccbrown
Copy link
Owner

ccbrown commented Jan 5, 2025

Ah, I see. I thought you were saying the link not acting like a link might be confusing. I'm fine with keeping as-is then. I was thinking most people would just want to keep the help forum disabled permanently, but I supposed they can just bookmark the nohelp URL for now.

I like the position of the link now and the loading indicator. Two minor things and then this'll lgtm:

  • Currently, the URL will have a hash like #page=&nohelp=true, #page=AGdgu2cAAYepuQ&nohelp=true, or #page=&nohelp=false. Can we minimize these to #nohelp, #page=AGdgu2cAAYepuQ&nohelp, and empty / no hash?
  • Can we add localization for the "Hide Help Forum" and "Show Help Forum" strings? For now, we can ask Gemini or ChatGPT what to use and I'll get someone to QC the translations at a later time. This'll mean those strings will need to be passed in through the HTML template in index_handler.go. Maybe just render them both in the HTML and keep one or the other hidden with display: none?

Thanks again!

@ccbrown
Copy link
Owner

ccbrown commented Jan 7, 2025

I'm going to go ahead and merge this as I don't consider either of those things strict blockers and they can be done in a separate PR. If you aren't able to take care of them, I'll likely knock them out some time this week and aim to get this deployed by the weekend.

Thanks for the PR!

@ccbrown ccbrown merged commit c503345 into ccbrown:main Jan 7, 2025
@abr-egn
Copy link
Contributor Author

abr-egn commented Jan 7, 2025

Thanks! Sorry, I likely won't get to those this week, that annoying gainful employment thing is monopolizing my time.

@ccbrown
Copy link
Owner

ccbrown commented Jan 11, 2025

Deployed! Thanks again!

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.

Stop tracking customer service threads
2 participants