Conversation
# Conflicts: # app/src/main/res/values/styles.xml
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.
Great idea regarding the snackbar for the feedback 💯
There are some required changes
private void promptToFavorite() { | ||
favoritesListView.setVisibility(GONE); | ||
progressBar.setVisibility(GONE); | ||
|
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 can remove this empty line
public void promptToSign() { | ||
favoritesListView.setVisibility(GONE); | ||
progressBar.setVisibility(GONE); | ||
|
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 can remove this empty line
@@ -92,8 +104,53 @@ public void onStop() { | |||
subscription.dispose(); | |||
} | |||
|
|||
public void updateWith(Schedule schedule, ScheduleViewPagerAdapter.OnEventClickedListener listener) { | |||
private void updateWith(ScheduledAndSignedIn scheduledAndSignedIn, ScheduleViewPagerAdapter.OnEventClickedListener listener) { | |||
if (scheduledAndSignedIn.hasPages()) { |
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.
This is leaking the fact that Schedule
contains pages
IMHO (I totally forgot about that TBH and it doesn't feel needed here).
I think that a isEmpty()
or hasSchedule()
would be better
.observeOn(AndroidSchedulers.mainThread()) | ||
.subscribe(schedule -> updateWith(schedule, this::onEventClicked)); | ||
} | ||
|
||
private BiFunction<Schedule, Boolean, ScheduledAndSignedIn> toScheduleAndLoggedInStuff() { |
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.
Stuff?
toScheduledAndSignedIn()
|
||
public class NoFavoritesView extends LinearLayout { | ||
|
||
private static final int MAGIC_NUMBER_TO_TRIGGER_ACHIEVEMENT = 5; |
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.
It is magic as in... magical ~ 💖
android:layout_height="wrap_content" | ||
android:tint="?colorAccent" | ||
android:src="@drawable/illustration_lunch" | ||
android:contentDescription="Sad picture of a panda" /> |
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.
Proper content description and extract to strings
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.
TBH if it's not important for a11y we should simply not set one (@ataulm might confirm here)
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_margin="12dp" | ||
android:text="We are sad and so should you" /> |
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.
Proper text and extract to strings
android:id="@+id/label" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_margin="12dp" |
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.
Extract to dimensions
android:layout_height="wrap_content" | ||
android:drawableTop="@drawable/illustration_prompt_to_sign_in" | ||
android:visibility="gone" | ||
android:foreground="@drawable/card_touch_feedback" |
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.
to style
@@ -0,0 +1,26 @@ | |||
<merge xmlns:android="http://schemas.android.com/apk/res/android" |
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.
Where is this layout used?
@alexstyl the build is failing:
Did you not push those changes? |
So confirmed
…On Wed, 29 Mar 2017, 10:14 Sebastiano Poggi, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/src/main/res/layout/merge_pretty_empty_view.xml
<#188 (comment)>:
> + android:id="@+id/empty_view"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:layout_gravity="center"
+ android:gravity="center_horizontal"
+ android:orientation="vertical"
+ tools:parentTag="LinearLayout">
+
+ <ImageView
+
+ android:id="@+id/illustration"
+ android:layout_width="wrap_content"
+ android:layout_height="wrap_content"
+ android:tint="?colorAccent"
+ ***@***.***/illustration_lunch"
+ android:contentDescription="Sad picture of a panda" />
TBH if it's not important for a11y we should simply not set one ***@***.***
<https://github.com/ataulm> might confirm here)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#188 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACjfG06C0odpg4-gAEFo2iQs7qODGZUSks5rqiEOgaJpZM4MsPtf>
.
|
|
||
setupToolbar(); | ||
|
||
if (!isInEditMode()) { | ||
Activity activity = unwrapToActivityContext(getContext()); |
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.
This block was moved to the !isInEditMode()
so that we can have a preview of this screen. As it turns out there is a specific kind of Context being used in the Preview mode and unwrapToActivityContext()
is crashing because it is unhandled. The specific Context is BridgeContext and looks like a Hidden API, so we cannot possibly handle it properly
# Conflicts: # app/src/main/res/layout/view_page_schedule.xml # app/src/main/res/values/styles.xml
This PR includes empty states for the favorite screen.
Screenshots
Before signing in
After signing in: