Skip to content
This repository has been archived by the owner on Apr 1, 2023. It is now read-only.

Add powered-by footer to event list, alternative to #63 #86

Merged
merged 1 commit into from Feb 1, 2017

Conversation

dmfs
Copy link
Contributor

@dmfs dmfs commented Jan 13, 2017

@lemonboston please review. We're going to add this to the list for now, but we're certainly going to add some behavior later on.

@dmfs dmfs requested a review from lemonboston January 13, 2017 15:41
@@ -12,9 +12,27 @@
android:layout_width="match_parent"
android:layout_height="match_parent"
app:layoutManager="LinearLayoutManager"
android:layout_marginBottom="20dp"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with layout inspector and this margin leaves a little bit too much space covered by the footer, can be changed to 24dp safely (I checked), to avoid incorrect proportions of the bottom items (no more events).

tools:context=".framework.eventlist.EventListActivity"
tools:listitem="@layout/schedjoules_list_item_event"/>

<include layout="@layout/sticky_header_layout"/>


<com.schedjoules.eventdiscovery.framework.microfragments.eventdetails.fragments.views.SchedJoulesFooterView
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the duplication should be resolved with the footer in the details view, either with layout of its own and include tag (extra tags can be added to include), or extracted style, but probably include is better, imo.

@dmfs
Copy link
Contributor Author

dmfs commented Jan 30, 2017

@lemonboston please review again. Note that I've rebased and squashed the commits, so you should remove your local branch first.
The problem was that the text size wasn't set to 0 which means the bar was slightly larger than the image+padding. Setting the textSize to 0 and increasing the padding fixed it.

@lemonboston
Copy link
Contributor

Why is SchedJoulesFooterView still a TextView? Couldn't it be simply an ImageView?

I think it would be useful to move the common layout definition of <SchedJoulesFooterView....> to a separate layout file and include it to remove the duplication. And use it like:

<include layout='schedjoules_powered_by_footer'
         app:layout_anchor="@id/schedjoules_footer_placeholder"
        app:layout_anchorGravity="bottom|center_horizontal"/>

So adding the special properties at the place of include.

@dmfs
Copy link
Contributor Author

dmfs commented Feb 1, 2017

If we use an ImageView we have to wrap it in a FrameLayout to get the padding and background color right. I agree that a TextView looks strange but it serves the purpose.
Good idea with the include. I'll do that.

@dmfs dmfs merged commit 53f9be1 into master Feb 1, 2017
@dmfs dmfs deleted the stories/63-powered-list branch February 16, 2017 21:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants