-
Notifications
You must be signed in to change notification settings - Fork 0
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
23 feat filtering for ride table #39
23 feat filtering for ride table #39
Conversation
…orm with the partial yet
…feat-filtersort-passenger-and-ride-information merged -- get latest styling changes from main
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 87.70% 87.84% +0.14%
==========================================
Files 31 31
Lines 374 395 +21
==========================================
+ Hits 328 347 +19
- Misses 46 48 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Everything looks awesome, need to add tests before merging.
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 good to me, more test coverage would be better!
btw this now has conflicts with main. I was gonna revert the changes there but I feel like resolving these here is better. |
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 good for current implementation.
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 much better, thank you for addressing the issues we discussed. Ideally we would be using FactoryBot for our testing (as opposed to just fully building test models on our own) but I think setting all that up can be a separate issue
Summary:
We should merge this to main for now and do the transfering over to the Datatables gem in issue #47