Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Couple of enhancements and changes #48

Merged
merged 1 commit into from Mar 6, 2018
Merged

Couple of enhancements and changes #48

merged 1 commit into from Mar 6, 2018

Conversation

flxwu
Copy link
Contributor

@flxwu flxwu commented Oct 29, 2017 •
edited

  • Replace ViewFiles TextView with much more flexible RecyclerView
  • Replace items with CardViews
  • Add ScreenUtils for converting DIP to pixels depending on screen size
  • Add ViewFilesDividerItemDecoration for decorating RecyclerView items
  • Change FilesAdapter to extend RecyclerView.Adapter<RecyclerView.ViewHolder>
  • Change ViewHolder to ViewFilesHolder for RecyclerView Adapter, declaring elements via Butterknife
  • Structured and Cleaned Classes and Code
  • Add Delete All Files option in file menu (will be changed to action bar option according to Delete Selected / Delete All files features. #47 )

@flxwu
Copy link
Contributor Author

flxwu commented Oct 29, 2017

@Swati4star This time, I even managed to squash the 4 commits without new conflicts! 😄 😆

@Swati4star
Copy link
Owner

The UI doesn't look good in view files section
device-2017-10-30-205257
The curved edges and a lot of space in between.

Regarding Delete All feature:
Add a confirmation dialog also. In case of deleting a single also, add a confirmation dialog.
You will be adding deleteAll feature in ActionBar in some other PR?

Swati4star
Swati4star reviewed Oct 30, 2017
View reviewed changes
app/src/main/java/swati4star/createpdf/adapter/ViewFilesAdapter.java Outdated
@@ -202,6 +236,29 @@ private void deleteFile(String name, int position) {

}

private void deleteAllFiles() {
Copy link
Owner

Choose a reason for hiding this comment

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

Add a comment on top of this function too.

@flxwu
Copy link
Contributor Author

flxwu commented Oct 30, 2017

@Swati4star What kind of item decoration would you prefer? Just a simple dividing line?

@flxwu
Copy link
Contributor Author

flxwu commented Oct 30, 2017

@Swati4star Regarding the deleting, I thought about swipe-to-delete as this would look much nicer and is relatively easy to implement with RecyclerView (one big advantage of RecyclerView compared to previous ListView)

@Swati4star
Copy link
Owner

Yes. Just a simple divider line. Almost like the previous one.

The swipe to delete will be for deleting one file. Right?
For multiple, you will add something like a selection button to action bar. And allow user to select them and delete. Or you have something else in mind?

@flxwu
Copy link
Contributor Author

flxwu commented Oct 31, 2017

Alright!
Another possible scenario would be "Files Actions" on long press and selection (and then the delete button will show up in action bar) on short press

@Swati4star
Copy link
Owner

User would prefer 'View File' to be easily available, as that must be the most used. That feature should not be on long press.

@flxwu
Copy link
Contributor Author

flxwu commented Oct 31, 2017

Just changed the ItemDecoration as you wanted.

Alright, so then swipe-to-delete for single and (action bar invoked) selection delete for multiple deletions.
I will work on the deletion as soon as you merge this.

@Swati4star
Copy link
Owner

So, this commit is a fix for #44

@flxwu
Copy link
Contributor Author

flxwu commented Oct 31, 2017

No, not directly. This is just an enhancement

@Swati4star
Copy link
Owner

Butterknife along with other enhancements.

@Swati4star
Copy link
Owner

device-2017-10-31-112446
UI still needs to be improved. The last divider line is different. Not a clean UI

@flxwu
Copy link
Contributor Author

flxwu commented Oct 31, 2017

Okay, will try to fix this... Any other adjustments?

@Swati4star
Copy link
Owner

No. just this enhancement.

@Swati4star
Copy link
Owner

Any updates on this issue? are you still working on this one?

Couple of enhancements and cleaning or structering changes
b4491a2 
-Replace ViewFiles TextView with much more flexible RecyclerView
-Replace items with CardViews
-Add ScreenUtils for converting DIP to pixels depending on screen size
-Add ViewFilesDividerItemDecoration for decorating RecyclerView items
-Change FilesAdapter to extend RecyclerView.Adapter<RecyclerView.ViewHolder>
-Change ViewHolder to ViewFilesHolder for RecyclerView Adapter, declaring elements via Butterknife
-Structured and Cleaned Classes and Code
- Replace ViewFiles TextView with RecyclerView
- Structure changes
- Code cleaning
- Change DividerItemDecoration design

- Replace ViewFiles TextView with RecyclerView
- Structure changes
- Code cleaning

Filelist items styling

Readme : added google play link

Bug fixes
@flxwu
Copy link
Contributor Author

flxwu commented Feb 1, 2018

Hey @Swati4star !
I actually stopped working on it, but as you just notified me, I came back to it and applied one last change and made the list styling as you wanted: Only a simple divider line.
I hope that you can merge it now! 👍 😄

@flxwu
Copy link
Contributor Author

flxwu commented Feb 1, 2018

I would work on swipe-to-delete as well if I find enough time

@flxwu
Copy link
Contributor Author

flxwu commented Feb 4, 2018

? @Swati4star 😄

@Swati4star
Copy link
Owner

hey sorry. I didn't look at this PR. I will surely review it within this week.

@flxwu
Copy link
Contributor Author

flxwu commented Feb 4, 2018 via email •
edited

@flxwu
Copy link
Contributor Author

flxwu commented Feb 13, 2018

Did you review it? @Swati4star

@flxwu
Copy link
Contributor Author

flxwu commented Feb 20, 2018

Aren't you going to review/merge this any time soon?

Swati4star
Swati4star reviewed Mar 6, 2018
View reviewed changes
app/src/main/res/layout/activity_main.xml
android:layout_height="match_parent"
android:fitsSystemWindows="true"
tools:openDrawer="start">
xmlns:app="http://schemas.android.com/apk/res-auto"
Copy link
Owner

Choose a reason for hiding this comment

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

why this indentation change?

@Swati4star Swati4star merged commit 4b6251b into Swati4star:master Mar 6, 2018
@Swati4star
Copy link
Owner

The PR looks great. I will merge it 👍
Thanks for such good work.

If you wish to work on swipe to delete, you can do that in another PR 👍

@flxwu flxwu deleted the recyclerView branch March 7, 2018 11:04
Swati4star pushed a commit that referenced this pull request Jul 4, 2018
@shiv07tiwari @Swati4star
fix #48 - added Image Compresson option (#161)
11c06e6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Swati4star Swati4star Swati4star left review comments

Assignees
No one assigned
Labels
need-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@flxwu @Swati4star