-
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
Settings page #45
Settings page #45
Conversation
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.
You don't need to leave in old stuff that works in case you've broken it, it clutters up the PR.
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.
It seems broken on my device.
- I can't read icons, it's white.
- Two 'debug' labels is suspicious.
- I don't see a 'change' button. Make sure it resizes properly.
- Show me a keypad if you're going to enforce only digits for support code.
- The screen goes blank when I try to type in something.
Regardless of the above, the priority is to finish IntroScreen for now. |
Cool re-request review when ready. |
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.
Nav bar labels show up in white, so I can't read it. Also, all the pages should have the same bg color so you update the others as well.
That's being done in 'making pages pretty' branch |
I can't see the pic you've uploaded, I think you pressed comment before waiting for it to upload? |
@saachipahwa Yep as I said you don't need the onChange/current postcode thing so I pushed a commit. The main reason it works is cos I call save() inside setState, so it redraws that widgets and therefore calls the future again as well. |
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, haven't looked at all the pages yet.
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.
In addition:
- I can't read icon labels (they are in white).
- Font size seems too big. (The text in wellbeing page gets cut off on my device, e.g. "- Normalized Ste|")
Should there be a back button on the checkup page, that takes them back to the app? or the bottom navigation bar |
I don't think so, it should close when they submit, and we sort of expect them to perform the checkup. |
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.
LGTM but review and merge #49 first.
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.
Can you resolve conflicts?
Also some updated screen shots at the top of the page would be good. |
the navigation bar at the bottom isn't showing so i hope i didn't break it (all i meant to do was add a new page).
(i renamed the old settings to testing and made a new settings page)