-
Notifications
You must be signed in to change notification settings - Fork 1
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
Es/device naming #73
Es/device naming #73
Conversation
0c6a1f7
to
f95b30f
Compare
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.
looking good! some blocking comments that may require changes
const TopoLinesWidth = 360; | ||
const TopoLineHeight = 640; |
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.
Mostly interested in adding original
somewhere, to make it clearer about they're representing. the screaming snakecase is just personal preference
const TopoLinesWidth = 360; | |
const TopoLineHeight = 640; | |
const TOPO_LINES_SVG_ORIGINAL_WIDTH = 360; | |
const TOPO_LINES_SVG_ORIGINAL_HEIGHT = 640; |
React.useEffect(() => { | ||
const unsubscribe = AppState.addEventListener('change', nextState => { | ||
if (nextState === 'background') { | ||
setDeviceName(deviceName); | ||
} | ||
}); | ||
return () => unsubscribe.remove(); | ||
}, [setDeviceName]); |
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.
what happens if you close the app at this point? my understanding is that this only persists the name when you leave this success screen.
I think the persistence should happen after you press the Add name
button in the device naming screen. only after that happens should you navigate to this screen. (and would remove the need for this effect entirely)
function handleAddNamePress() { | ||
if (invalidName) { | ||
setErrorTimeout(); | ||
return; | ||
} | ||
|
||
navigation.navigate('Success', {deviceName: name}); | ||
} |
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.
mentioned in other comment, but i think the persistence of the device name should happen here and you should only navigate to the success screen after that happens without error
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.
additionally, saving the device name requires making a call to the backend (we have a setDeviceInfo()
method on the manager), so worth keeping that in mind here in case you want to mock something for the time being.
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 a semi annoying complication with conditional rendering of screens. If we set the name here the name is set, and these screen will not be rendered anymore (including the next screen). The conditional rendering is nice because it is declarative, aka the screens are based of the state of the device Name.
We could make it imperative, and set the name here, and just navigate to the success screen, and then navigate to the map screen. It would just require the app to imperatively check the state on every open of the app. Aka, on the open of the app we would have to do the following at the top level:
// imperative
const deviceId = getDeviceInfo()
if (!deviceId) {
navigate("deviceNaming)
} else {
navigate("map)
}
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.
Also if we do it imperatively, after the hit go to map
, they are navigated to the map. But if they hit the back button, those screen will be on the nav stack, so they would be able to go back to those screen. We could do a check on those screens:
// in all device naming screens
useLayoutEffect(()=>{
if(!!deviceId) navigate("map")
})
that feels equally as clunky as the AppState Event listener
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.
hm i see what you mean. i think the effects are what concern me here, and I think we should be updating this state based on a specific interaction (such as the add name button in this screen, or go to map button in the other screen).
thoughts on this approach? (using basic useState just for simplicity):
// In practice, this will actually come from the backend
const [deviceName, setDeviceName] = useState(() => {
// initial value comes from wherever it's persisted
})
const [needsDeviceNamingFlow, setNeedsDeviceNamingFlow] = useState(() => {
// initialize based on initial `deviceName` state. if it's defined, then this should be false
// this won't updated when deviceName changes since it's the initializer
return !deviceName
})
// Then in the render...
{needsDeviceNamingFlow ? <DeviceNameScreens onFinish={() => setNeedsDeviceNaming(false)}} /> : ...}
so the onFinish()
here would be called in the success screen.
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.
just a note that the device name will live on the backend (see the setDeviceInfo()
and getDeviceInfo()
methods on MapeoManager) so i think ideally this isn't using the persisted state setup. using react query seems more aligned with managing the device information state
Adds Screens which allow user to name their device.
Conditionally renders the device naming screens if device name does not exist (held in persisted storage), and renders home screens if the name does exists
To Do:
Screen Shots
Closes #1117