-
Notifications
You must be signed in to change notification settings - Fork 0
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
V2 posture monitor #359
V2 posture monitor #359
Conversation
fixes a bug in our current version that required extra styling to align the sliderthumb
Reviewed 4 of 20 files at r1. app/components/Button.js, line 55 at r1 (raw file):
Rename those style properties so they're consistent with the variable names app/components/posture/PostureMonitor.js, line 67 at r1 (raw file):
How come this is -25? app/components/posture/PostureMonitor.js, line 87 at r1 (raw file):
I assume the
app/components/posture/postureMonitor/CircularProgress.js, line 6 at r1 (raw file):
Can this be a stateless component instead of a full-blown React Component? Comments from Reviewable |
Reviewed 3 of 20 files at r1. app/components/Button.js, line 55 at r1 (raw file): Previously, kevhuang (Kevin Huang) wrote…
Done. app/components/posture/PostureMonitor.js, line 67 at r1 (raw file): Previously, kevhuang (Kevin Huang) wrote…
app/components/posture/postureMonitor/CircularProgress.js, line 6 at r1 (raw file): Previously, kevhuang (Kevin Huang) wrote…
can't pass functional components to Comments from Reviewable |
Reviewed 1 of 2 files at r2. app/components/posture/PostureMonitor.js, line 67 at r1 (raw file): Previously, miguelc1221 (Miguel Correa) wrote…
But why is it -25 and not -30 like the comments and other lines? app/components/posture/postureMonitor/CircularProgress.js, line 6 at r1 (raw file): Previously, miguelc1221 (Miguel Correa) wrote…
Thanks Comments from Reviewable |
Reviewed 16 of 20 files at r1, 2 of 2 files at r2. app/components/posture/PostureMonitor.js, line 67 at r1 (raw file): Previously, kevhuang (Kevin Huang) wrote…
ah my bad, didn't notice. yea it should be -30. Fixed app/components/posture/PostureMonitor.js, line 87 at r1 (raw file): Previously, kevhuang (Kevin Huang) wrote…
Done. Comments from Reviewable |
Reviewed 13 of 20 files at r1, 1 of 2 files at r2, 1 of 1 files at r3. app/components/posture/PostureMonitor.js, line 76 at r3 (raw file):
Please rename to app/components/posture/PostureMonitor.js, line 92 at r3 (raw file):
Please rename to app/components/posture/PostureMonitor.js, line 94 at r3 (raw file):
Can we just return app/components/posture/PostureMonitor.js, line 1104 at r3 (raw file):
Remove the app/components/posture/postureMonitor/Monitor.js, line 20 at r3 (raw file):
Set app/components/posture/postureMonitor/Monitor.js, line 27 at r3 (raw file):
Swap the array members. We should use the convention of defining defaults (i.e., app/components/posture/postureMonitor/Monitor.js, line 44 at r3 (raw file):
Let's remove the recalibration button for now. We probably won't have the firmware updated in time to support this. Sorry. app/styles/button.js, line 31 at r3 (raw file):
Should we add app/styles/monitorButton.js, line 19 at r3 (raw file):
app/styles/theme.js, line 25 at r3 (raw file):
Use #4CAF50 (Material green 500) app/styles/posture/postureMonitor.js, line 1 at r3 (raw file):
This is how it looks on my Pixel: The dots are mispositioned (see my comments about that) and the button text are not in view. For the button text to be in view, you may need to set the container's app/styles/posture/postureMonitor.js, line 6 at r3 (raw file):
Since
app/styles/posture/postureMonitor.js, line 43 at r3 (raw file):
app/styles/posture/postureMonitor.js, line 44 at r3 (raw file):
app/styles/posture/postureMonitor.js, line 46 at r3 (raw file):
app/styles/posture/postureMonitor.js, line 67 at r3 (raw file):
app/styles/posture/postureMonitor.js, line 72 at r3 (raw file):
app/styles/posture/postureMonitor.js, line 78 at r3 (raw file):
Apply to app/styles/posture/postureMonitor.js, line 80 at r3 (raw file):
Try applying the following across both iOS and Android:
app/styles/posture/postureMonitor.js, line 98 at r3 (raw file):
Try applying the following across both iOS and Android:
Comments from Reviewable |
Review status: all files reviewed at latest revision, 21 unresolved discussions. app/components/posture/PostureMonitor.js, line 94 at r3 (raw file): Previously, kevhuang (Kevin Huang) wrote…
yup app/styles/button.js, line 31 at r3 (raw file): Previously, kevhuang (Kevin Huang) wrote…
sure app/styles/posture/postureMonitor.js, line 80 at r3 (raw file): Previously, kevhuang (Kevin Huang) wrote…
40 and 25% didn't work for iphone 6 and 6+ and my android device. I used instead,
app/styles/posture/postureMonitor.js, line 98 at r3 (raw file): Previously, kevhuang (Kevin Huang) wrote…
used 44 and 24% Comments from Reviewable |
Reviewed 1 of 20 files at r1, 6 of 7 files at r4. app/components/posture/PostureMonitor.js, line 1104 at r3 (raw file): Previously, kevhuang (Kevin Huang) wrote…
There's another app/styles/monitorButton.js, line 1 at r4 (raw file):
Please move this file under app/styles/posture app/styles/monitorButton.js, line 8 at r4 (raw file):
app/styles/posture/postureMonitor.js, line 80 at r3 (raw file): Previously, miguelc1221 (Miguel Correa) wrote…
Let's try to move away from the arbitrary percentages for Now that we have the radius (hypotenuse) and the angle (60 degrees), we can calculate the length of the side opposite of the angle. See http://www.mathportal.org/calculators/plane-geometry-calculators/right-triangle-calculator.php for an example. Can you try the following for the two circles?
I'm not too sure about subtracting 1/8th of the circle's width (the 15 / 8 part) at the end. When I didn't subtract the 1/8th, there was a minor gap between the arc and the dot. Comments from Reviewable |
Reviewed 1 of 7 files at r4. app/styles/monitorButton.js, line 37 at r4 (raw file):
Would 8 be okay? 14 seems too far apart. app/styles/monitorButton.js, line 38 at r4 (raw file):
Let's remove this font size override and just use the default font size from app/styles/posture/postureMonitor.js, line 114 at r4 (raw file):
Instead of applying these vertical margins, how about wrapping the
Comments from Reviewable |
a discussion (no related file): Comments from Reviewable |
Reviewed 6 of 7 files at r4. app/styles/monitorButton.js, line 37 at r4 (raw file): Previously, kevhuang (Kevin Huang) wrote…
sure looks fine to me app/styles/posture/postureMonitor.js, line 80 at r3 (raw file): Previously, kevhuang (Kevin Huang) wrote…
damn, schooling me right now lol. The above code works fine on my end. Comments from Reviewable |
Reviewed 1 of 21 files at r1, 10 of 13 files at r5. app/styles/posture/postureMonitor.js, line 80 at r3 (raw file): Previously, miguelc1221 (Miguel Correa) wrote…
Nice! I'm glad that worked on your end too. I checked on an Android tablet, and it looks fine too. Comments from Reviewable |
Posture Monitor
I refactor the monitor to reflect the new design. The monitor scene currently doesn't have a title on the title bar. I am thinking we going to pass a
title prop
to the titlebar when navigating to the posture monitor? I used https://github.com/bgryszko/react-native-circular-progress for the arc of the monitor. Though I had to fork it and merge some PRs in order to get some functionality we needed, similar to how we did with the video-player. I also updated thereact-native-slider
, since it fixed an issue that we were manually fixing ourselves (thumb was not inline with the slider).Make sure to add "react-addons-shallow-compare" to your dependencies. I wasn't sure if to add it to this PR
Testing
postureCalibrate
orpostureMonitor
scene. The time is all messed up because we are not passing in a session time yet.This change is