smacgregor / InstagramClient

Android Instagram application that shows a list of popular images on instagram
0 stars 0 forks source link

Week 1 Project: Instagram Photo Viewer #7

Open smacgregor opened 8 years ago

smacgregor commented 8 years ago

My app is complete, please review. /cc @codepathreview @codepath

Looking for feedback in particular on the layout for my listview cell: item_photo.xml

I used three layouts:

I'd like to learn if I could have been more efficient.

Issues Encountered / Interesting Notes

XML includes and Butterknife

I wanted to use XML includes in item_photo.xml to break that UI down into smaller re-usable components. But butterknife crashes if I try to include a layout and give that layout an id like:

<include layout="@layout/item_comment"
    
   android:id="@+id/tvFirstComment"

   android:layout_below="@+id/tvCaption"
    
   android:layout_width="match_parent"
    
   android:layout_height="wrap_content"
    
   android:paddingTop="5dp"/>

It can't find a field with the id tvFirstComment

Resizing the blue heart icon

I grabbed an image off the web that was too large. I had to resort to using Picasso to resize the image down to what I wanted. This is inefficient.

        Picasso.with(getContext()).load(R.drawable.small_like_heart)
                .resize(20, 0)
                .into(viewHolder.likeHeartImageView);

Should I have been able to do that in the xml without involving Picasso? I struggled to get the right scaleType that would scale it to 20px without distortion or adding white space around the image.

GSON

The GSON library was great - I was easily able to construct all of my models without parsing the JSON response directly. One downside is the model hierarchy has to exactly match the JSON hierarchy for everything to work.

styles.xml

I overrode the default theme with custom colors that matched instgram's color palette and that worked well. But I was trying to figure out where to put generic padding constants. For instance, many of the elements in a list view cell for an instagram post have the same left and right padding (20dp) values. I wanted to put that value in one place and then refer to it by name in the layout.

nesquena commented 8 years ago

I wanted to use XML includes in item_photo.xml to break that UI down into smaller re-usable components. But butterknife crashes if I try to include a layout and give that layout an id like:

This is to be expected as include tags don't actually exist in the layout, they simply insert the layout specified. In other words, you shouldn't try to find the include tag by id. Instead, look up the id of the views within the @layout/item_comment as if they were in the layout file directly.

Resizing the blue heart icon

You should probably have resized the image outside of the context of Android here. It's much more efficient to resize the image to the desired size using "Preview" or any image editor. This is especially important given you should be resizing images for all densities anyways.

GSON library was great, match the JSON hierarchy for everything to work.

Yeah, GSON can be very convenient however, the fact that the models need to match the JSON hierarchy can make things way more complex than necessary in some cases. Although this can be customized by changing the mappings manually. In other cases, it adds more complexity to the model structure then it's worth, so use your best judgement.

I wanted to put that value in one place and then refer to it by name in the layout.

Using dimens.xml is a best practice. Hardcoding dimensions in the layout is good for small projects but is often an anti-pattern.

smacgregor commented 8 years ago

This is to be expected as include tags don't actually exist in the layout, they simply insert the layout specified. In other words, you shouldn't try to find the include tag by id. Instead, look up the id of the views within the @layout/item_comment as if they were in the layout file directly.

Ah - that makes sense. Although it makes it hard to build a layout from re-usable components using include files if the id is specified in the include. i.e. if I have an include that specifies the layout for a comment - I can't include the same xml multiple times if I want multiple comments in a layout since they will all have the same id.

Using dimens.xml is a best practice. Hardcoding dimensions in the layout is good for small projects but is often an anti-pattern.

Well let's squash that anti-pattern before it gets started. https://github.com/smacgregor/InstagramClient/pull/8 moves various values into colors.xml and dimens.xml.

Thanks for the tips and insights.

nesquena commented 8 years ago

Ah - that makes sense. Although it makes it hard to build a layout from re-usable components using include files if the id is specified in the include. i.e. if I have an include that specifies the layout for a comment - I can't include the same xml multiple times if I want multiple comments in a layout since they will all have the same id.

Yes you are correct. Instead the reusable components you are looking for are called Fragments and we will be looking at these soon.

codepathreview commented 8 years ago

:+1: Excellent work! Good job getting most of the stories completed.

A few notes after checking out the code:

One of the most important part of these projects is that you add additional features and tweak the UI / UX because that will provide many more learning opportunities. I would encourage you to complete the projects each week with required stories early and then spend time adding your own UI elements and experimenting with optional extensions that will improve the user experience.

We have provided a detailed Project 1 Feedback Guide here which covers the most common points we see for this project. Read through the feedback guide point-by-point to determine other ways you could improve your submission. You should consider going back and implementing applicable feedback as well. Keep in mind that one of the most important parts of Android development is learning the correct patterns and conventions.

Hopefully this first project has given you a better sense of working with RelativeLayout which is a very flexible layout system, probably one of the most powerful responsive-first layout systems available across web and mobile platforms. This assignment also gave us our first introduction to networking, working with APIs and loading remote images. The next assignment will introduce new concept such as accepting user input and navigating between activities but will also reinforce important concepts such as networking, using APIs, handling remote images.

If you have any particular questions about the assignment in general or on any of the feedback, feel free to reply here or or email us support@codepath.com.

P.S. Good to see you properly added the readme with features and screenshots to your project as described in the submitting assignments guide!

smacgregor commented 8 years ago

Thanks so much for the feedback Nate!

Consider using the ViewHolder pattern to improve performance of photos ListView.

the view holder pattern was implemented here: https://github.com/smacgregor/InstagramClient/blob/master/app/src/main/java/com/smacgregor/instagramclient/viewing/InstagramPostsAdapter.java#L29

See you at class tonight.