-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add the locker tab #161
Add the locker tab #161
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.
Good PR, there are some some changes I suggested but were are getting there 👍// TODO
comments and
EDIT: didn't notice PR's comment about TODO comments
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 took another look at this PR, I'd really like locker_tab.dart
to be split into separate widgets, each private method that now renders subtree could be extracted to new Widget, eg. AppsSheet
and AppsSheet.showModal
, FacesSheet
, AppsItem
, FacesCard
etc.
Otherwise PR looks good and I also saw #162 was merged. 👍
Yup, I'm gonna work on that, though it's a little bit of work so be patient :P |
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.
Most of my comments touch upon code's style in new widgets, implementation is fine otherwise.
Following the mockups, leaving a lot of parts as dead ends, to be implemented later (all should be marked with TODO)
Fix #35
Depends on #162