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

move isAutoTimeAndDate / isAutoTimeZone from react-native-device-info #65

Merged
merged 4 commits into from
Sep 8, 2019

Conversation

mikehardy
Copy link
Contributor

Summary

Howdy @zoontek :-) - this module (which I use and which is fantastic by the way) already has time and date information in it and does it well

react-native-device-info also had some duplicate time and date functions, plus these 2 which were unique

I'd like to have one place in the react-native-community for this type of information and this seems like a natural home

So I'm purging them out of react-native-device-info in preference to here

There was no iOS implementation of them in react-native-device-info (maybe it is not possible? unknown) so this is the complete implementation, such as it is

I did not add it to the changelog as requested in this template because I could not see a changelog?

Test Plan

I added it to the Sync and Async example. Seems to work? Although getting the example to work with local code changes required me to run yarn add "file:../" before it would pick up changes...

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS
Android

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I mentioned this change in CHANGELOG.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)

debug.keystore is in the .gitignore from react-native init, but it
is required for the example to run as-is on android
These are from react-native-device-info - this module already handles timezone
and locale things very well, and seems like a natural home for these things within
the react-native-community versus having time/date information split across packages
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
example/src/AsyncExample.js Show resolved Hide resolved
@mikehardy
Copy link
Contributor Author

Thanks for considering this and after you think on it I will of course make whatever changes you like so the code is comfortable over here. I'm doing a little research on iOS APIs too, maybe we can actually have an implementation for iOS who knows?

@mikehardy
Copy link
Contributor Author

Okay, I searched through the API docs from Apple, and couldn't find anything. I asked the original author of the API here in case they know react-native-device-info/react-native-device-info#583 (comment) - but I think this is simply not available on iOS, so once you decide exactly how you want to specify returns for platform-specific stuff I'll shape this up for a second review. Thanks @zoontek

@zoontek
Copy link
Owner

zoontek commented Sep 5, 2019

@mikehardy I would rather go with:

type Option<T> = T | undefined

usesAutoDateAndTime(): Option<boolean>
usesAutoTimeZone(): Option<boolean>

I find it less confusing 🙂

@mikehardy
Copy link
Contributor Author

got it! I will do exactly that. Also the author of the original PR researched and came to the same conclusion I did - there is simply no API for iOS for these, so we're not missing anything - the PR is multi-platform "complete" such as it is. I'll tag you when I've pushed and feel like this is ready. Thanks!

@mikehardy
Copy link
Contributor Author

Okay @zoontek I think it's ready for a look again, here are the changes I made - some of which were in existing code, all with the idea of API-similarity:

  • I added the Option<boolean> type you asked for, *and I updated your existing boolean-APIs to also return it (I think you'll like that? But I could be wrong)
  • I altered these two APIs to be called usesXXX instead of isXXX, because your other boolean-APIs started with uses, not is (Again, I think you'll like that? But I could be wrong)

Let me know what you think!

@zoontek
Copy link
Owner

zoontek commented Sep 8, 2019

Looks pretty good except that uses24HourClock and usesMetricSystem returns a boolean 100% of the time on all platforms, so no need to wrap them in a Option 😅

EDIT: I can merge it and make the changes if you want

@mikehardy
Copy link
Contributor Author

@zoontek I just pushed that up with the original two methods untouched, and just my proposed additions as the 'Option' :-)

@zoontek
Copy link
Owner

zoontek commented Sep 8, 2019

Perfect, I merge it :)

@zoontek zoontek merged commit 445f048 into zoontek:master Sep 8, 2019
@mikehardy
Copy link
Contributor Author

Wait! I always re-re-read things (can't help it) and I noticed I still had index.js with the Option wrapping in the function definitions, sorry! easy fix at least

@mikehardy mikehardy deleted the auto-time-date branch September 8, 2019 17:55
@zoontek
Copy link
Owner

zoontek commented Sep 8, 2019

@mikehardy I will cleanup that before the release, no problem

@zoontek
Copy link
Owner

zoontek commented Sep 10, 2019

@mikehardy
Copy link
Contributor Author

Sweet! Thank you so much @zoontek - react-native-device-info v3 will be out shortly then I think

@mashad6
Copy link

mashad6 commented Feb 28, 2022

Is it gonna be on IOS any time soon?

@mikehardy
Copy link
Contributor Author

@mashad6 just as soon as someone figures out 1) if it is possible then 2) proposes a PR to implement it I think. Could be as soon as today if you put in the effort and do it, in my experience @zoontek reviews + merges + releases things really fast when people put in the effort to contribute

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