-
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
Improved styling the landing screen #5772
Conversation
@alyblenkin please take a look and let me know what you think. Here as well the landscape mode looks worse than what we already have on master so it might require some tweaks. |
Please make sure to also take a quick look at languages like French that have longer taglines! @alyblenkin, I see you did it in mockups but would be good to see how you feel about it on a real device. |
Even on master there wasn't enough space for such long translations and now that text is even bigger so it takes even more space. The problem is that those are two separate strings |
Good to know. I think the text was very small before and challenging to read. I think it is worthwhile refactoring it so we can display it on two lines if it's not too much work. |
6e12431
to
0822600
Compare
Done. |
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 this is a great next iteration! Next time we redesign the landing screen we will likely have a new logo :)
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've got one idea for an improvement we could make while we're here so will hold off on "needs testing" for now.
collect_app/src/main/java/org/odk/collect/android/activities/FirstLaunchActivity.kt
Outdated
Show resolved
Hide resolved
1c73359
to
cf34ae0
Compare
collect_app/src/main/java/org/odk/collect/android/activities/FirstLaunchActivity.kt
Outdated
Show resolved
Hide resolved
07904a5
to
beb3ae0
Compare
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.
beb3ae0
to
560770b
Compare
560770b
to
b507b04
Compare
collect_app/src/main/java/org/odk/collect/android/activities/FirstLaunchActivity.kt
Outdated
Show resolved
Hide resolved
2cbcbc0
to
56629a6
Compare
private val projectsRepository: ProjectsRepository, | ||
private val projectsDataService: ProjectsDataService | ||
) : ViewModel() { | ||
private val _isLoading = MutableLiveData<Boolean>() |
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 make this nullable?
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.
Because I don't want to throw false as the initial value. That would mess up what we have above in the activity class plus when it comes to the progress dialog it also does not need to receive that initial false value.
And one more thing in kotlin it's not nullable because the data type is Boolean
not Boolean?
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.
Ah because you need to differentiate between "no longer loaded" and "hasn't loaded yet" for the observation that launches the Main Menu. It's strange because the LiveData
is technically nullable (you could set it to null
) due to some weirdness in the generic typing which was the whole reason to introduce the NonNull...
implementations.
Here I guess this all works without a null check (again in the observation that launches the Main Menu) because _isLoading
has no "initial" value. However, I think if you called getValue
on it, you would get back null
. Given that, I'd prefer to use MutableLiveData<Boolean?>(null)
and then add a null check in so that it's clearer what's happening. It looks like the newest lifecycle
modules actually give us correct typing for nullable types.
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 already added a null check in MaterialProgressDialogFragment because it's java but you think it would be good to do the same here:
viewModel.isLoading.observe(this@FirstLaunchActivity) { isLoading ->
if (!isLoading) {
ActivityUtils.startActivityAndCloseAllOthers(
this@FirstLaunchActivity,
MainMenuActivity::class.java
)
}
}
when observing it can't be null you said that it's possible to get null by calling getValue()
but if I'm not using that method I don't think if it makes sense to change anything.
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.
Ok I realized I was being over cautious because of how LiveData
used to work. I thought I'd be able to write the following line in FirstLaunchViewModel
:
_isLoading.value = null
This used to be possible, but it is now fixed (a lint check fails). I knew they'd improved this, but hadn't realised we'd finally got protection on the setter side as well as in the Observer
.
I think this means that we can actually deprecate our custom NonNull
types!
Tested with Success!
|
Tested with Success! Verified on Androids: 12,13 |
Closes #5733
Why is this the best possible solution? Were any other approaches considered?
This is what we have discussed in the issue and in this pr.
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?
Testing the landing screen will be enough since nothing else has been touched. Changes that I have added:
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.