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

2520: Show number search results #2976

Merged
merged 19 commits into from
Nov 14, 2024
Merged

Conversation

simomps
Copy link
Contributor

@simomps simomps commented Oct 25, 2024

Short description

Show the amount of search results

Proposed changes

  • Add counter in the SearchPage
  • Use searchResultsCount from translation.json

Side effects

None.

Testing

Check css styling

Resolved issues

Fixes: #2520


Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, definitely going in the right direction already and working as expected 🎉
I added a few minor comments, please have a look at styled components.
Implementing this for native is still missing, let me know if you feel comfortable addressing this already, otherwise I can also take over.

One additional thing: We tend to add release notes for issues that are user facing to improve our store presence. Could you please add one for this change? Have a look here: https://github.com/digitalfabrik/integreat-app/blob/main/docs/contributing.md#release-notes

JFYI: The fact that the build is failing is not your fault, to avoid this, simply rebase your branch to main or merge main into your branch. Can also be done at a later point though.

web/src/routes/SearchPage.tsx Outdated Show resolved Hide resolved
web/src/routes/SearchPage.tsx Outdated Show resolved Hide resolved
@simomps simomps marked this pull request as draft November 6, 2024 13:15
@simomps simomps marked this pull request as ready for review November 6, 2024 15:00
Copy link
Member

@ztefanie ztefanie left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. 🚀

Tested on Web and Android, looks already really good. 👍

I just added some small comments that should be easy to fix.

web/src/routes/SearchPage.tsx Outdated Show resolved Hide resolved
web/src/routes/SearchPage.tsx Outdated Show resolved Hide resolved
web/src/routes/SearchPage.tsx Outdated Show resolved Hide resolved
native/src/routes/SearchModal.tsx Outdated Show resolved Hide resolved
@simomps
Copy link
Contributor Author

simomps commented Nov 9, 2024

Thank you for the review 🌸

I think i resolved all issue suggestions :))

@simomps simomps requested a review from ztefanie November 9, 2024 13:17
Copy link
Member

@ztefanie ztefanie left a comment

Choose a reason for hiding this comment

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

Nice! Thank you for the adjustments. Looks good 🚀

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Nice, looks great. Tested on firefox and real android, works as expected 😍

You still have to run prettier as this fails the checks (the formatting in the release note is somehow off):

yarn prettier:write

One other thing: The ticket also mentions to add this to the city search (i.e. http://localhost:9000/landing/de) on both web and native, could you still add the same functionality there? If not, thats okay as well, then we can just do a separate PR for it.

Copy link
Member

@steffenkleinle steffenkleinle left a comment

Choose a reason for hiding this comment

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

Nice work, looks great! Tested on firefox and android

@ztefanie
Copy link
Member

Please squash the commits when you are finished, buy clicking "Squash and merge" here:

@simomps simomps merged commit eeeaf57 into main Nov 14, 2024
9 checks passed
@simomps simomps deleted the 2520-show-number-search-results branch November 14, 2024 13:09
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.

Show number of search results
3 participants