-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improving empty states #5768
Improving empty states #5768
Conversation
@alyblenkin please take a look at the changes I have implemented. I haven't added icons because I don't have them and I don't think I can download them from FIgma? If we want to add those icons could you share them here? |
Hey @grzesiek2010 - It's looking good! Could we use a medium font weight for the title, and then in the second line, we use a regular font weight to establish more of a visual hierarchy? I think we could also use the same font size as the main menu buttons because we have the space. You can download the icons from figma, but I think using Google fonts is easier! I'm using the same edit icon as "Drafts" and send icon that we are using for "Ready to send". The only one that is new is the empty inbox for "Sent" Side note: every time I go to open "Delete from" the app crashes for some reason. |
What about other lists for example the list of blank forms, what icon should be used there? |
ooooo great point! Adding a mockup for that now. |
d9acbc0
to
6e40004
Compare
6e40004
to
2f9de4d
Compare
2f9de4d
to
7368fc0
Compare
@alyblenkin it should be ready for another look. |
@grzesiek2010 Looks great! |
@@ -1,5 +1,7 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<resources> | |||
<attr name="icon" format="reference" /> |
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.
What's this for?
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.
The icon attribute is used in two places and if it's not extracted like that Android studio throws Found item Attr/icon more than one time
see: https://stackoverflow.com/questions/4434327/same-named-attributes-in-attrs-xml-for-custom-view or https://medium.com/@bruno.lima/android-custom-views-same-attribute-on-multiple-custom-views-78e9de1ce6c8
android:layout_width="match_parent" | ||
android:layout_height="wrap_content" | ||
android:textAppearance="?textAppearanceLabelExtraLarge" | ||
android:fontFamily="sans-serif-medium" |
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.
Is there another text appearance we could use instead of specifying a font here?
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.
Not really but maybe textAppearanceLabelExtraLarge
should use that font family by default? textAppearanceLabelLarge
does so textAppearanceLabelExtraLarge
should as well I guess?
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.
Not really but maybe textAppearanceLabelExtraLarge should use that font family by default?
Maybe! I do wonder if we should be using one of the title tokens here rather than a label one as they're typically for controls (docs here).
@alyblenkin what do you think? |
It's a good question - the only two icons I didn't want to make consistent with the main menu are "Start new" and "Sent". For Start new form, I wanted to show a form icon to illustrate the text a bit more. For Sent, I didn't want someone to glance at the empty state and see a checkmark and be confused, so I used the empty inbox. For download, we should use the same icon. |
Fixed. |
Tested with Success! Verified on device with Android 13 Verified cases:
|
Tested with Success! Verified on device with Android 10 |
Closes #5730
What has been done to verify that this works as intended?
I've tested the changes manually and added some new tests/updated existing ones.
Why is this the best possible solution? Were any other approaches considered?
Nothing to discuss here.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
We can focus on testing lists (their empty states). Nothing else should be changed.
Do we need any specific form for testing your changes? If so, please attach one.
No.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.