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 new filters to "list" method: maxDate, minDate #38

Merged
merged 4 commits into from
May 12, 2019

Conversation

OlivierFreyssinet-old
Copy link
Contributor

No description provided.

@mikehardy
Copy link
Collaborator

These look fantastic. Could you update 2 things to help users so they can use this (cool) new feature?

The readme chunk about filters: https://github.com/briankabiro/react-native-get-sms-android/blob/master/README.md#L77

If I understand correctly this filter is not exclusive like the other 4, in other words you can apply a date range regardless of any other filters, including pagination? But I'm not sure what the date format should look like (is it milliseconds since unix epoch? something else?), so I would be confused personally even though I could use this feature in a work project right now and I'm motivated :-)

Maybe add the use of them to the example? https://github.com/briankabiro/react-native-get-sms-android/blob/master/example/App.js#L107

The second item in particular - the example - doesn't seem as important but it is how I will check it locally, so if the example demonstrates the feature it makes me feel much more confident 👍ing for merge

@OlivierFreyssinet-old
Copy link
Contributor Author

Thanks for your super quick feedback! I'll do this as soon as possible (this weekend ideally).

The new filters work like this:

  • The date is indeed milliseconds since unix epoch. I will add this to the readme too.
  • If and only if you set a maxDate, it's like executing this SQL query SELECT * from messages WHERE /*other filters*/ AND date <= maxDate.
  • If and only if you set a minDate, it's like executing this SQL query SELECT * from messages WHERE /*other filters*/ AND date >= minDate.
  • The pagination (indexFrom and maxCount) is done after applying these filters, so I think these filters shouldn't cause anything weird.

Please tell me if it's not clear or if you think it might be a breaking change.

@mikehardy
Copy link
Collaborator

I don't think this is breaking at all, just looks like a really useful new feature. That documentation would also serve well - it seemed clear to me.

@mikehardy
Copy link
Collaborator

Looks great - thanks for the readme update - we don't have CI so I need to pull this locally and test it but these seem okay visually. Great contribution

@OlivierFreyssinet-old
Copy link
Contributor Author

Cool!
If you wait a bit I can still implement a small example, just didn't have time yet.

@mikehardy
Copy link
Collaborator

That would be great - I'll get my env set up locally to pull your branch but will look for an update from you before final check and +1 on merging

@briankabiro
Copy link
Owner

Great work both on getting this ready for merging. The code looks good.

Will be on the lookout for the update and then we can get this live!

@mikehardy
Copy link
Collaborator

@OlivierFreyssinet hey Olivier - no pressure as I understand this is open source :-) - but curious if you had time to add a quick use of the new filter to the example? I like this change and would love to merge it in. If you don't have time I understand, just let us know and we'll figure something out to test it for merge

@OlivierFreyssinet-old
Copy link
Contributor Author

OlivierFreyssinet-old commented May 9, 2019

Hi, sorry I totally forgot about this, so I just added a quick use of the new filters to the example. I also tried to clean up a bit the example UI as it would become super messy with this new stuff... you will judge, it's not my biggest success 😂I just thought inputs should look like inputs and touchables like buttons...
The easiest way to test is to copy the timestamp of one message, paste it in the maxDate or minDate input & refresh the list.

@mikehardy
Copy link
Collaborator

mikehardy commented May 9, 2019

Hey this is fantastic - I am not going to tell you the example UI is the best looking thing I have seen, but it works - I tried a time-filter search window, and narrowing the window and I consistently got the exact results I expected, and easily using the copy/paste buttons. For an example demonstrating the functionality is everything.

I'm +1 on this - the code looked good and it works, with nice features in the example

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Tested -> works

@briankabiro
Copy link
Owner

Super! Think we can merge it then.

I will update here when I make the release.

@briankabiro briankabiro merged commit abeb894 into briankabiro:master May 12, 2019
@briankabiro
Copy link
Owner

Heya. I've pushed this and it's now on npm under 1.4.1 as the latest version!

Sidenote: I couldn't do it with 1.4.0 as I found out that I had mistakenly pushed the tag a year ago.

Thanks for the good work and the contribution, @OlivierFreyssinet. Thanks for taking the time to review this and get it going, @mikehardy.

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.

3 participants