-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Setting to choose coordinate display format #526
Conversation
…etween LatLon, UTM, DMS, DD
I don't feel super qualified to review this, but it looks good to me! If it all seems to be working in your manual tests, I think it's ok to merge now. cc @gmaclennan to take a look over when he's back. |
Hi! I'm back! Great work on this @luandro! I haven't installed this or checked the code yet, but based purely on the screenshots a couple of comments:
|
Also we should put this data (selected coordinate system) in React Context, and the hook should read from there. I can talk through the reasons for this and how to do this on our next check-in. |
lol thanks, was really confused that both looked exactly the same.
Makes a lot of sense.
👍
In relation to what comes first (lat or long): My interpratation: Lat/Long is commonly used because Latitute was discovered first, as has been used first since. Long/Lat is also commonly used because of conventional math (ie, f(x ,y, z)). I think lat/long will make more sense for the user, while long/lat will make more sense for a machine. Thoughts @gmaclennan, as you have more experience? @rudokemper might also have some thought on this. |
In my experience, latitude is generally always written out first, conforming to your observations on user expectations @luandro. But that might be specific to the contexts where I have done GIS work (Suriname, Brazil, Colombia, and now Kenya). But even experienced GIS specialists get lat & long confused all the time, and have to look up which is which, haha.. |
Todo:
Forgetting anything @gmaclennan? |
Had a bit of a hard time updating my knowledge on React hooks, but all worked out. Ready for another review @gmaclennan. |
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.
Adding my feedback here that I shared over Slack, so there's a record of it. I've fixed this up myself and I'm just testing locally before pushing.
const defaultContext = [initialState, () => {}]; | ||
const SettingsContext = React.createContext(defaultContext); | ||
|
||
function reducer(state: State, action: Action): State { | ||
switch (action.type) { | ||
case "set": { | ||
return action.value; | ||
} | ||
case "set_coordinate_system": { | ||
try { | ||
return { ...state, coordinateSystem: action.value }; | ||
} catch { | ||
return state; | ||
} | ||
} | ||
default: | ||
return state; | ||
} | ||
} | ||
|
||
const getData = async dispatch => { | ||
try { | ||
// AsyncStorage.clear() | ||
const state = await AsyncStorage.getItem(STORE_KEY); | ||
if (state) { | ||
const parsedState = JSON.parse(state); | ||
dispatch({ type: "set", value: parsedState }); | ||
return parsedState; | ||
} else return initialState; | ||
} catch (e) { | ||
console.log("Failed to fetch the data from storage"); | ||
return initialState; | ||
} | ||
}; | ||
|
||
const saveData = async value => { | ||
try { | ||
const stringified = JSON.stringify(value); | ||
await AsyncStorage.setItem(STORE_KEY, stringified); | ||
return value; | ||
} catch (e) { | ||
console.log("Failed to save the data to the storage"); | ||
} | ||
}; | ||
|
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.
There is already a usePersistedState
hook, so best use that rather than have two different implementations of this.
}, [contextValue]); | ||
return ( | ||
<SettingsContext.Provider | ||
value={{ settings: contextValue[0], dispatch: contextValue[1] }} |
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.
This creates a new object on every render
src/frontend/lib/utils.js
Outdated
const formattedSeconds = Number( | ||
Math.round(seconds + "e" + decimals) + "e-" + decimals | ||
); |
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.
Why not seconds.toFixed(3)
?
@@ -55,6 +58,7 @@ const LocationField = ({ children }: Props) => { | |||
return children({ | |||
longitude: value.lon, | |||
latitude: value.lat, | |||
system: coordinateSystem, |
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.
This doesn't really belong here, because this component has nothing to do with the display of coordinates. To separate concerns, I think it's better to keep this in the <LocationView>
component
* develop: feat: Setting to choose coordinate display format (#526) chore: Re-apply c267a99 with correct npm@6 version and lockfile v1 Revert "feat: Updated translations for default configuration (vi, srn)" feat: Updated translations for default configuration (vi, srn) fix: Update Thai, Khmer & Vietnamese translations feat: Add Dutch translations feat: Add Sranan Tongo translations feat: Add French translations fix: Fix cutoff of text on OnePlus6T phone #502 (#511) fix: Write preset tags to observations (#514) chore: Fix Github CI builds by removing NDK r22 (#516) fix: Fix import config button crash (#512) feat: Updated translations (vi, es, po) (#498) # Conflicts: # src/backend/package-lock.json # src/backend/package.json # src/frontend/screens/Settings/Settings.js
This adds the ability to change the coordinate system, saving to AsyncStorage the selected option.