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
Keeping filters state on pagination #638
Keeping filters state on pagination #638
Conversation
@GPrimola thank you for this pull request! I will be taking a look at this closely when I get the time. I'll be looking at the code, loading it in Docker, and seeing if the CI can be made to pass testing. I might not be able to get to this right away, but this is a really great addition, so I hope to be able to merge it soon. Thanks again. Update: I see the CI is "not passing" due to CodeClimate. I personally don't mind CodeClimate warnings very much. It may have helpful suggestions, but what is more important to me is the app working as well as it can. Coding style is a distant second or third priority, in my opinion. |
I can say as an initial reaction: I am happy to see the filters are part of the URL parameters! Thank you for that! |
@DeeDeeG I'm glad to contribute to this project, specially because I identify with the community as a gay man. :) About Code Climate's complain, the thing is that I touch a Regarding code style, I totally understand your feeling, because they don't add value to the end-user. However, as this is a OSS, I strongly recommend to have some linters, like Rubocop, due to the amount of developers that write for this code base. With a linter you can have a standardized code base. I'd like to ask you to add Thank you!! :) |
Doesn't sound difficult to add. Hopefully it will not disrupt things too much for me -- I'm the most-active maintainer, but I didn't write most of the code. I'm probably the least-expert programmer who has been a maintainer, heh. I'm best at non-code things, like setting up Docker, handling merges with Git, and researching/updating dependencies. So reviewing a lot of small code changes might be a bit much for me to handle personally. I do my best, though! Maybe it would work out fine. It does sound like somewhat of a good idea. I see there was some intent to do this way back in #165.
Ah, I see. Can do. I found this tweet https://twitter.com/hacktoberfest/status/1312221208667185153 and article https://hacktoberfest.digitalocean.com/hacktoberfest-update explaining it in more detail. (That honestly doesn't surprise me, as handling the increased volume of Pull Requests well is a lot of work! Sad to hear about the actual spam, though.) Anyhow, thanks for letting me know about this. Adding that topic for a month is no problem. |
Hi again. I was able to take a look at this in Docker, and in a Heroku instance (Heroku is the hosted web app platform we use). This looks pretty good overall. I am seeing a bug, though. Sometimes, clicking a filter button fails to toggle that filter. The page refreshes, but the same filters are used as from before the refresh. (Tested in Firefox and Chrome, Ubuntu and macOS, from Docker and on Heroku). (This is not 100% of the time. It often gets "stuck" with the same set of filters enabled, unable to change which ones are on or off through multiple clicks and page refreshes. Sometimes, waiting about 10 seconds seems to "un-stick" the filters and make them changeable again, but this also doesn't work 100% of the time.) I don't see an obvious cause, so at the moment I am writing this comment, I can't be of much help figuring out why this happens. I noticed it happens fairly often when: all three filters are enabled, and I'm trying to toggle one of them off. (But again, this is not 100% consistent). I hosted a separate Heroku app on my account for testing purposes. In case that makes it more convenient to test this, I will link it here: https://refuge-deedeeg-test.herokuapp.com/restrooms?utf8=%E2%9C%93&lat=37.7749295&long=-122.4194155&search=san+francisco |
@DeeDeeG I'll take a look on that! I just tried at the link you sent and could notice it too. |
Hey @DeeDeeG I'll push updates on this by tomorrow! Sorry to be off in the last days! |
@DeeDeeG could you test it again? I just pushed new code. |
@GPrimola very nice! This looks like it is working 100%! Sorry it took me so long to look at this updated code. You can try it again at the updated test Heroku instance: https://refuge-deedeeg-test.herokuapp.com/restrooms?utf8=%E2%9C%93&lat=37.7749295&long=-122.4194155&search=san+francisco I re-ran the Travis CI tests (automated functionality testing) and it's passing now. There are some warnings that are cluttering the test output, though.
(From: https://travis-ci.com/github/RefugeRestrooms/refugerestrooms/builds/189750688#L940) Is there any way to make sure the typical operation of the site doesn't generate this warning in the console? I think we could live with that, but ideally there should be a way to not hit that warning. Thanks again for your efforts on this. - DeeDeeG |
@GPrimola I don't know if you saw in my comment above, but there is a warning in the console with this Pull Request. jQuery.Deferred exception: undefined is not a function (evaluating 'Object.entries(filters)') http://127.0.0.1:43067/packs-test/js/application-742f5a6fbd25fb94965c.js:864:17 Do you have any thoughts on how to fix it? |
Hey @DeeDeeG ! Thanks and sorry for that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks.
(Updated again at https://refuge-deedeeg-test.herokuapp.com/restrooms?utf8=%E2%9C%93&lat=37.7749295&long=-122.4194155&search=san+francisco if anyone wants to check it out. Works fine for me. Also, there are no more warning in Travis CI. 👍)
Thanks for this, I'd say it's ready to merge!
Update: This is now live at our staging Heroku instance (which tracks our develop
branch): https://staging.refugerestrooms.org/
* Finish Upgrading Webpacker to v5 (#637) - Gemfile[.lock]: Update webpacker to 5.2.1 - Webpacker: Update config files Ran "rails webpacker:install" and committed the changes. In "config/webpacker.yml," kept the ".js.erb" entry under "extensions:" so that "app/javascript/packs/lib/maps.js.erb" is included. In "config/webpack/environment.js," kept jQuery and rails-erb-loader configs, as we are still using these. - JS dependencies: Upgrade webpack-dev-server Commit the changes from running `rails webpacker:install`, but using a caret "^" semver range for "@rails/webpacker", rather than an exact version. (So that we get updates in the 5.x series) * Update dependencies for September 2020 (#636) - dependencies: Remove `swagger` node module We don't actually use this. Also removes 200 sub-dependencies, and makes our yarn.lock file about 1270 lines shorter! - yarn.lock: Upgrade bootstrap to 4.5.2 (Was at version 4.4.1) - dependencies: Resolve node-fetch to ^2.6.1 - yarn.lock: Upgrade "selfsigned" and "node-forge" - Gemfile[.lock]: Update Rails and dependencies rails was 5.2.4.3, now it's version 5.2.4.4 * Keeping filters state on pagination (#638) - Keeping filters state on pagination - Fix Code Climates complaints. - Fixed active issue on ada filter buttons - Fixed undefined function problem when run rspec. * Add rubocop and resolve lint errors (#644) - Initialize rubocop - Run rubocop with --fix - Don't require magic comment This will affect every file and have potential side effects, so I'm going to start with this turned off. - Resolve lint errors in `app` - RSpec linting - Resolve remaining - Fix wrong change - Singleton method for verify - New lint rules - Add rubocop to travis config - Correct docker compose command - Check for geo The condition was actually supposed to be `if geo = results.first`, because that's not always obvious the intention and fails lint, I'm just doing a truthy check for it which should be the same. - Fix typo - Add contributing docs for rubocop * Applying ADA filters for the Map view of search. (#651) - Applying ADA filters for the Map view of search. - Changed comment for polyfill URLSearchParams - Fixed bug of map marker content not showing. * Add a standard EditorConfig configuration for every contributor to follow (#649) * Enhancement/i18n thread safe (#647) - Provide a way to respond requests without thread-safe issues As rails docs says https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests When we use 'I18n.locale =' the current locale could leak into following requests this is a standard and recommended way to deal with it :) - Create a shared_example to test I18n locale switching - Update spec/controllers/pages_controller_spec.rb * Upgrade puma to latest version 5.x (#641) * yarn.lock: Update "http-proxy" to v1.18.1 (fe195d7) Co-authored-by: Lucas <torres.giorgio@gmail.com> Co-authored-by: Tegan Rauh <3896310+tegandbiscuits@users.noreply.github.com> Co-authored-by: Bruno Casali <brunoocasali@gmail.com>
Context
Summary of Changes
apps/javascript/packs/views/restrooms/restrooms.js
filters
query parameter and sent to the backend.restrooms_filters
to whitelist the given filters;Checklist
Screenshots
Before
Page 1
Page 2
After
Page 1
Page 2