-
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
Final UI changes #122
Final UI changes #122
Conversation
@@ -44,15 +45,18 @@ public DeleteItemsFragment(DeleteCallBack deleteCallBack, ArrayList<Item> select | |||
@Override | |||
public Dialog onCreateDialog(@Nullable Bundle savedInstanceState) { | |||
View view = getLayoutInflater().inflate(R.layout.fragment_delete_items, null); | |||
TextView dialogMessage = view.findViewById(R.id.dialogMessage); | |||
dialogMessage.setText("This will permanently remove the selected items from your inventory."); |
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 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!
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.
Jared this PR looks so good. Thanks for all the hard work, buddy, LGTM!
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.
Thanks again for making all these fixes @jdrco. Super clutch. A couple nitpicks from me:
- Spacing between Details and Tags sections seems pretty big
- If an item doesn't have a comment, we should probably conditionally hide the Comment header/section, to reduce the amount of empty space there, using
VISIBILITY=GONE
- It might just be me, but the toolbar icons seem to have went from too big to too small. Maybe a size of 24-26dp would be better
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.
@MatthewNeufeld does this look good for fixing the Tag button alignment? Please re-approve if so |
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.
thanks @jdrco for addressing those, it looks great!
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.
Looks great - big shoutout to you @jdrco. LGTM!!!!!!!!!
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.
@jdrco I would buy this app because of how clean the UI looks. Great job everyone!
Describe your changes
This PR accomplishes the following: