alisonthemonster / Presently

Android app for recording gratitude journal entries -- over 1 million installs, contribute today!
388 stars 81 forks source link

Add ViewPager to Entry view #261

Closed SEAbdulbasit closed 2 years ago

SEAbdulbasit commented 2 years ago

:scroll: Description

I have added another Fragment for ViewPager. First, we retrieve all the entries and submit them to ViewPager. Then we find the selected date and move to that page position. Converted parentFragmentManager to supportFragmentManager I had difficulty popping up the view pager fragment.

Ticket: https://github.com/alisonthemonster/Presently/issues/161

:green_heart: How did you test it?

:camera_flash: Screenshots / GIFs

https://user-images.githubusercontent.com/33172684/188437796-9e31e05c-6489-44c4-ad06-d8ef5960e3e0.mp4

SEAbdulbasit commented 2 years ago

While I was creating the video, another use case popped up in my head, what if we edit and then swipe ? As data will be lost, Is it okay or should we also should a dialogue on swipe incase of edit ?

alisonthemonster commented 2 years ago

Thank you so much for adding this feature!! Apologies I didn't get to review it sooner, I've kicked off the CI builds and I'm reviewing now :)

While I was creating the video, another use case popped up in my head, what if we edit and then swipe ? As data will be lost, Is it okay or should we also should a dialogue on swipe incase of edit ?

That's a great point! I think a dialog in the case where they've typed and swipe away sounds perfect!

SEAbdulbasit commented 2 years ago

Regarding this https://github.com/alisonthemonster/Presently/pull/261#issuecomment-1241430757 Showing the dialog,I was looking into some solutions. One solution might be to implement an interface at EntryView and then get the current fragment at the view pager and check if any edits were made when swiping. If edits were made, then we will cancel the swiping and show a dialogue. We cannot check at EnteryView because when swiped, the page will be changed and dialogue will be showing at the previous or next entry. Would love to hear your thoughts on this solution.

SEAbdulbasit commented 2 years ago

@alisonthemonster Thanks for the comments. I have addressed the comments and looked into the failing tests. I forgot to update the EntryViewPagerViewModelTest which is now done.

alisonthemonster commented 2 years ago

We are so close to getting this landed! 76.55% is the current method coverage and we need 77!

There are a few changes we can make to bump up coverage: In EntryViewPagerFragment I think we can make adapter and previousPosition both private fields. In ViewPagerAdapter the var numEntries and the val fragment can be private too.

But I think with using Mavericks and Jacoco we're just going to run into problems moving forward. I'm going to go ahead and get rid of the method coverage requirement. I think we're sufficiently tracking code coverage using line/instruction/class. I'll get that merged and then you can merge/rebase and we should be good to go!

SEAbdulbasit commented 2 years ago

@alisonthemonster you can run the coverage task. I have pulled the changes.

alisonthemonster commented 2 years ago

Thank you for your patience! I'll get this merged and work on a release :)

SEAbdulbasit commented 2 years ago

@alisonthemonster found a case. Incase of new entry, if swipe, will stuck in a loop as entry does not get reset. Looking into it.

https://user-images.githubusercontent.com/33172684/196130140-f5d182ce-9d76-486d-9916-1ae856bb71a7.mp4