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

🐛 fix search state bug & missing last result #21417

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

cathysarisky
Copy link
Contributor

@cathysarisky cathysarisky commented Oct 25, 2024

closes #21343

This fixes an issue in sodo-search where early results (for example, matching the first letter typed in the search box) would get "stuck" in the display, due to failure to update state (or actually, lacking state).

Converted PostItems to a component, and gave paginatedPosts state.

I also fixed an undocumented bug where the last search result didn't appear, due to an error in with slice's register.

@cathysarisky cathysarisky changed the title fix search state bug & missing last result 🐛 fix search state bug & missing last result Oct 26, 2024
@allouis allouis merged commit 967cf23 into TryGhost:main Oct 28, 2024
18 checks passed
Copy link
Member

@kevinansfield kevinansfield left a comment

Choose a reason for hiding this comment

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

@cathysarisky the use of state and useEffect added here is a React anti-pattern. Preferred approach is to use posts to build paginatedPosts directly in the first render rather than going the round-about way of using an effect to update state and causing a second render 🙂

If building the list directly in the first render is causing problems then we have a different issue somewhere that needs addressing.

@kevinansfield
Copy link
Member

Ugh, sorry, I'd missed how we're using maxPosts state here and how posts changing resets the number of posts shown - this is the problem with useEffect for things like this, it makes the flow much harder to follow!

Seems like there's a refactor waiting here somewhere but your fix looks good for now 🙏

@cathysarisky cathysarisky deleted the search-quirk-21343 branch October 29, 2024 17:45
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.

Quirky search behavior
3 participants