-
Notifications
You must be signed in to change notification settings - Fork 35
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
1428/entropy screen changes #1446
Conversation
This reverts commit f8d10cd.
@@ -31,35 +27,38 @@ const styles = StyleSheet.create({ | |||
bigFont: { | |||
fontSize: Typography.text4XL, | |||
}, | |||
contentView: { |
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.
A lot of these keys are duplicated in the style definition for the Container
component.
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.
Addressed in a commit.
const styles = StyleSheet.create({ | ||
handPosition: { | ||
// TODO change to be % based | ||
top: -29, |
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 does this need to be % based? Because magic numbers seem hack-y?
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.
I think as long as there's a comment explaining that this is the position where the hand fits in the sparkly bit, it should be fine.
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.
Tested on iPhone 5S device, iPhone XR simulator.
Also run quickly by Liz, she seemed happy with it.
@@ -26,7 +26,8 @@ interface State { | |||
} | |||
|
|||
// we are gonna collect some from the user and the rest from the OS | |||
const ENOUGH_ENTROPY_PROGRESS = 0.6 | |||
const ENOUGH_ENTROPY_PROGRESS = 0.3 | |||
const POST_COLLECTION_WAIT_TIME = 1000 |
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.
I feel that this wait time seems a bit unnatural - I'm not sure the best course of action though. Instantaneous is also a bit jumpy.
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.
Ideally the navigation would be a bit smoother. I don't think it's a matter of the timeout so much, and rather the animation used during navigation (or in this case the lack of one). Not entirely sure how to approach this / what the best course of action is.
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.
We disabled the animations/transitions between the starting screens - this is probably a casualty of that. I would suggest lowering the timeout for now and then iterating on solutions to make it smoother in the future.
There are some magic numbers in the handAnimation file that place the finger in the appropriate place relative to the other svg. A comment has been added to explain what these are for.
Waiting for 1000ms seemed a bit unnatural. The period has been reduced but in the future there should be a smoother transition - some kind of animation.
// will start the loop and clean up when it's lifetime is over | ||
useEffect(() => { | ||
const timeout = setTimeout(pulse, 0) | ||
return () => clearTimeout(timeout) |
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.
Late to the party, but this will not work because the timeout has already fired. The recursive callbacks (pulse
) need to have a stop flag. Will resolve in #1417
implements #1428 but requires feedback regarding the exact text, layout and transition effect the use