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

Refactor location status checks #749

Merged
merged 26 commits into from
May 7, 2020

Conversation

dahliasalem
Copy link
Contributor

Linked issues:

Screenshots:

How to test

Copy link
Contributor

@tstirrat tstirrat left a comment

Choose a reason for hiding this comment

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

Thank you thank you!!!

app/Entry.js Outdated Show resolved Hide resolved
app/services/LocationService.js Show resolved Hide resolved
app/services/LocationService.js Show resolved Hide resolved
app/services/LocationService.js Outdated Show resolved Hide resolved
app/views/LocationController.js Outdated Show resolved Hide resolved
app/views/LocationController.js Outdated Show resolved Hide resolved
app/services/LocationService.js Outdated Show resolved Hide resolved
app/services/LocationService.js Show resolved Hide resolved
app/services/LocationService.js Outdated Show resolved Hide resolved
app/services/LocationService.js Outdated Show resolved Hide resolved
@dahliasalem dahliasalem marked this pull request as ready for review May 4, 2020 18:44
Copy link
Contributor

@tstirrat tstirrat left a comment

Choose a reason for hiding this comment

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

🎉

app/views/LocationController.js Outdated Show resolved Hide resolved
app/views/tracking/ExposurePage.js Outdated Show resolved Hide resolved
app/views/tracking/MayoInfo.js Outdated Show resolved Hide resolved
import MayoInfo from './MayoInfo';
import styles from './style';

const buttonLabel = languages.t('label.home_enable_location');
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite. languages export is a legacy thing. it exports i18next which also has a t but that T wont be fully reactive.

The useTranslation hook will re-render if language is changed. vs languages.t will update if re-rendered by some other means only. Because it's not reactive, it wont trigger a re-render on language change.

app/views/LocationController.js Outdated Show resolved Hide resolved
app/views/LocationController.js Outdated Show resolved Hide resolved
app/views/LocationController.js Outdated Show resolved Hide resolved
tstirrat
tstirrat previously approved these changes May 4, 2020
Copy link
Contributor

@tstirrat tstirrat left a comment

Choose a reason for hiding this comment

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

Looks good:

  • Put behind a feature flag
  • Add unit tests for the new methods in LocationService.js

app/views/tracking/ExposurePage.js Outdated Show resolved Hide resolved
app/views/tracking/OffPage.js Outdated Show resolved Hide resolved
app/views/tracking/UnknownPage.js Outdated Show resolved Hide resolved
tstirrat
tstirrat previously approved these changes May 6, 2020
@tstirrat tstirrat changed the title Refactor location Refactor location status checks May 6, 2020
@kenpugsley kenpugsley self-requested a review May 7, 2020 06:20
Copy link
Collaborator

@kenpugsley kenpugsley left a comment

Choose a reason for hiding this comment

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

Looks great! I did a pass through the code (looks good) and did a round of functional testing. The only issue I might raise is that we should probably have a state for "no health authority", but I don't think we should hold this up for that.

@tstirrat
Copy link
Contributor

tstirrat commented May 7, 2020

The only issue I might raise is that we should probably have a state for "no health authority", but I don't think we should hold this up for that.

Yeah, that's being tracked here: https://pathcheck.atlassian.net/browse/SAF-85

@aitkeni
Copy link

aitkeni commented May 10, 2020

Awesome!

aputinski pushed a commit to safe-paths-contrib/covid-safe-paths that referenced this pull request May 11, 2020
* official/develop:
  Fix EULA modal close icon color (Path-Check#792)
  feat(i18n) Language updates, May 10 (Path-Check#790)
  feat(GPS) Store Location Data in an encrypted Realm DB (Path-Check#788)
  Enable location status checks in testing build (Path-Check#787)
  fix(Onboarding) Fix EULA links to open in device browser (Path-Check#783)
  chore(Build) Fix debug build error with apache commons-io lib (Path-Check#775, SAF-210)
  chore(Testing) snapshot tests for LocationTracking,  main/* (Path-Check#785)
  Only render mayo clinic link when language is english (Path-Check#782)
  fix(UX) Fix pulse alignment on Android devices with “notch” (Path-Check#771)
  fix(Location) Improve location "tracking" status checks (Path-Check#749)
  Fix text crop in About screen (Path-Check#774)
  [iOS] add missing languages (Path-Check#773)
monaabd27 pushed a commit to monaabd27/covid-safe-paths that referenced this pull request May 18, 2020
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.

User is not notified if location is not enabled after onboarding
5 participants