search

dynamite8 / resources-android-dev

Collection of resources for becoming an Android developer
7 stars 7 forks source link

Fixes #61: moved UI classes to the UI package #72

Closed oa2013 closed 6 years ago

oa2013 commented 6 years ago

Fixes #61. Hey @JaeW can you review this PR, please? I just moved the UI classes to a separate package. Nothing new added.

JaeW commented 6 years ago

@oa2013 Looks awesome! @dynamite8 is this a case where the commits need to be squashed?

oa2013 commented 6 years ago

@JaeW or @dynamite8 -- the last part of this commit adds a CustomView. I'm beginning to nest fragments :( see comments in the code.

JaeW commented 6 years ago

@oa2013 in the StickeyNoteView class, I am not seeing any public accessors where the text can be set and retrieved on StickeyNoteView object instances. It appears that the only place it can pull strings from is the resources folder, and once that string is set there's no way to change it.

Nice job on recycling the TypedArray in the finally block, btw. That would have been very easy to miss.

JaeW commented 6 years ago

@oa2013 This tutorial explains how to make component view attributes accessible (create getters and setters). https://android.jlelse.eu/custom-views-make-your-android-app-stand-out-fa386b506860

dambadipudi commented 6 years ago

@oa2013 the ResourceListAdapter is a Recycler View adapter class. Would it be appropriate to move that to the ui package as well?

JaeW commented 6 years ago

@dambadipudi You asked @oa2013 about this, but I wanted to let you know that in a lot of Google-authored repositories I have seen an 'adapter' package within the 'ui' package so as to keep things tidy. In the end there's no absolute right or wrong way to organize a project - whatever makes the system more logical and understandable is right.

oa2013 commented 6 years ago

@JaeW @dambadipudi @dynamite8 this PR is getting very big and I just had to resolve a merge conflict for MainActivity...can someone please review and merge? It closes a bunch of issues. @dambadipudi yes, I can move the Adapter into the UI package. I'd like to merge the PR first and make sure everything works.

JaeW commented 6 years ago

@oa2013 sorry. I was waiting for @dynamite8 on this, but I can go ahead and merge it. I will leave it to you to go ahead and make the changes we talked about in the meetup this Sunday.

dambadipudi commented 6 years ago

Nice work on converting the svgs to pngs! You seemed to have changed the implementation for the bottom navigation toolbar as well. I also see change in code from basic layout to creating CustomViews.

I do have a question regarding Issues and PR. All the above seem to be unrelated to "moving UI classes to UI package" right? Isn't it better if each small fix (say converting svg vector asset files to png files, for e.g.) has it's own Issue and PR ?

EDIT - Just noticed that they all have different commit messages related to different issues. Then why are they all showing up under 1 PR?

I also have some questions regarding the new InnerResourceFrag.java file for displaying resources by topic. Is there a need to create another fragment for this? I think the main activity fragment can send an Intent with the topic to the existing ResourceFragment. In ResourceFragment, if a topic is provided in the Intent, it can display resources by that topic. Or else, i can display all resources.

dambadipudi commented 6 years ago

@JaeW both ui folder and an adapter folder under ui folder are great ideas 👍 I just didn't want it left outside the ui folder :)

oa2013 commented 6 years ago

@dambadipudi

About the bottom toolbar changes: I created the Main Fragment so that it can act as a container and house the Home Fragment and the InnerResourceFrag. So when the application first starts, we see the Main Fragment which adds the Home Fragment and when a sticky note is clicked, the Home Fragment is replaced by the InnerResourceFrag.

I thought about using the existing ResourceFragment but wanted to make sure that the Main Fragment -> Home Fragment --> InnerResourceFragment switching worked smoothly first without breaking the bottom view pager.

Do you think we should create an issue replace the InnerResourceFragment with the existing ResourceFragment?

Yes, it's a good idea to resolve one issue in each PR. I will try my best to do that :-)

dambadipudi commented 6 years ago

Thank you for the explanation about the navigation. I do have a question regarding the current flow. Would the bottom View Pager stay at "Home" even after the sticky note is clicked and InnerResourceFragment is shown?

oa2013 commented 6 years ago

No and that's a problem -- right now, the only way to get back to the Home Fragment is by using the back button on the device.

The view pager needs to be dynamically updated when the InnerResourceFrag is created and I've had a lot of issues with that.

That, by the way, may be a good reason to do go from Home Fragment --> ResourceFragment via Intent as you suggested. I think that way we are not adding anything new to the viewpager.