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.

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

New design reader bottom sheet - step3 #11193

Merged
merged 21 commits into from Feb 5, 2020

Conversation

develric
Copy link
Contributor

@develric develric commented Jan 30, 2020

Fixes #11016

NOTE: this requires PR #11183 and #11184 merged first into develop

image image

Notes

  • The feature is active in the zalpha flavor.
  • While working on this one found an issue that can happen mostly with sites which number of subscribers can vary fastly (like for example longreads.com). Since the method ReaderBlog.isSamecompares also number of subscribers by default, it could happen that the subfilter in the bottom sheet could remain not selected even if it was the current selected subfilter (since a changing in the number of subscribers would made the isSame test to fail). Introduced an isSamemethod that allows comparison skipping number of subscribers.
  • Moved the logic to refresh/load the subfilters from the bottomsheet tabs fragments (SubfilterPageFragment) to the root bottomsheet fragment (SubfilterBottomSheetFragment) since it seems a more suitable place.

To test

Test case 1: Empty state not visible when following sites and tag

  • Follow some sites and tags if not already
  • Open the filter bottom sheet and check both SITESand TAGS shoes the list of followed tags and sites and not the empty states

Test case 2: Empty state visible for SITES when no sites are followed

  • Follow some sites and tags if not already
  • Open the filter bottom sheet and check both SITESand TAGS shoes the list of followed sites and tags and not the empty states
  • Unfollow all sites and check that the SITES tab shows the empty state as per above image
  • Tap on the FOLLOW A SITE cta and check that the followed sites management tab gets opened

Test case 3: Empty state visible for TAGS when no tags are followed

  • Follow some sites and tags if not already
  • Open the filter bottom sheet and check both SITESand TAGS shoes the list of followed sites and tags and not the empty states
  • Unfollow all tags and check that the TAGS tab shows the empty state as per above image
  • Tap on the ADD A TAG cta and check that the followed tags management tab gets opened

Test case 4

Since we made modifications to the add_content_bottom_sheet that is used also in the main create fab bottom sheet, makes sense to tap on the main FAB in My Site and smoke test the bottom sheet.

Test case 5

Compile and run vanilla flavor, smoke test the standard reader and the main create fab bottom sheet.

PR submission checklist:
note: not adding accessibility here since there will be a dedicated audit of the feature to close ticket #10962

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 30, 2020

You can test the changes on this Pull Request by downloading the APK here.

@develric develric added [Status] Needs Design Review A designer needs to sign off on the implemented design. [Type] Enhancement IA Information Architecture Reader labels Jan 30, 2020
@develric develric added this to the 14.2 milestone Jan 30, 2020
@develric develric changed the title ew design reader bottom sheet - step3 New design reader bottom sheet - step3 Jan 30, 2020
Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

Looking good, thanks. Specific to this PR I only have one tweak/clarification

Empty state actions 'ADD A TAG'
Expected: using text button style
Actual: type size looks a little small - can you confirm you're using the text button component for this?

@develric
Copy link
Contributor Author

type size looks a little small - can you confirm you're using the text button component for this?

You have good eyes @osullivanchris 😊! You are right I was using a simple TextView actually like the SWITCH SITE one in My Site. Changed to Borderless Button and the result is as below. Even if I need to say it looks pretty similar dimension-wise (using style="@style/Widget.AppCompat.Button.Borderless" that I think defaults text size to 14sp (the size I was using for the TextView also), eventually we can increase it). In any case makes sense to use the button and I will change that one once we decide on the styling/size.

Before After
image image

@osullivanchris
Copy link

Looks pretty much the same to me but semantically more correct to use a button so lets do it!

@develric
Copy link
Contributor Author

Hi @osullivanchris, changed the TextViewinto a Button. Here below how it appears for final reference:

@osullivanchris
Copy link

LGTM!

@develric develric added [Status] Needs Code Review and removed [Status] Needs Design Review A designer needs to sign off on the implemented design. labels Feb 3, 2020
@malinajirka malinajirka self-assigned this Feb 3, 2020
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks @develric! I've done my first review pass. I've left a couple of questions and comments. Let me know what you think;).

@develric
Copy link
Contributor Author

develric commented Feb 4, 2020

Hey @malinajirka 👋! Should be ready for a second pass! Let me know, thanks 😊.

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks @develric!

I've tested the app and it looks great:)!!! It's very smooth and easy to use. Good job to you both @develric and @osullivanchris! I've found only two minor issues.

  1. The fully expanded state is lost during rotation.
  2. Possible UX issue: When a site/tag is selected and I open the bottom sheet again and I click on the selected site/tag, I'd expect to de-select it. cc @osullivanchris

It seems we weren't on the same page when we discussed changing the method names so they follow the convention @khaykov mentioned in his review. It seems you added on prefix to the method names. However, imo it's not the core of the "issue". Ideally the view layer would just inform the VM what happened instead of telling it what to do.

Nice example is one of the onClickListeners

mSubFiltersListButton.setOnClickListener(v -> {
                mViewModel.setIsBottomSheetShowing(true);
                mViewModel.onChangeBottomSheetVisibility(true);
            });

We might want to consider replacing both the mViewModel.xyz with a single statement mViewModel.onSubFiltersListButtonClicked(). It's the VM's responsibility to perform the correct actions. Same thing applies to several other method invocations through out the PR.

One of the really nice things about this approach is, that it facilitates writing unit tests and they are more readable and they have better documentation value. The unit tests don't call any specific action methods, but only call actions of the user/system. So for example, instead of testing setIsBottomSheetShowing(true) works as expected, we test bottom sheet shown when user clicks on sub filters list button -> we invoke vm.userClickedOnSubFiltersListButton in the test and verify the visibility state has changed to the expected state (we basically simulate a user click). Does it make sense or should I elaborate more?

Having said that I feel like this whole fragment isn't prepared for a clean VM usage and would need a lot of refactoring to get there. Some of the methods can't be properly named as we are actually telling the VM what to do as the business logic is present in the view layer. I'd try to rename some of the methods, but don't worry about all of them.
You haven't introduced this inconsistency in this fragment, but I'll mention it to make sure we are all on the same page. In general, imho we should try to keep VMs as clean as possible. So when we are modifying a legacy code with business logic in the view layer, sometimes it's better to put more logic into the view layer than to make the VM architecture not-clean (not necessarily directly into the Activity/Fragment, we can still encapsulate in a new class). On the other hand if there is a part of logic which can be extracted into a VM and we keep the architecture clean, it's better to put it into the VM to facilitate future refactoring of the legacy code. Wdyt?

adapter.update(items)
viewModel.updateTabTitle(category, items.size)
readerViewModel.onUpdateTabTitleCount(category, items.size)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment to the one above.

Copy link
Contributor Author

@develric develric Feb 4, 2020

Choose a reason for hiding this comment

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

Changed the name method to onSubfilterPageUpdated here 89dab37

@osullivanchris
Copy link

osullivanchris commented Feb 4, 2020

@malinajirka thanks for testing and for the positive feedback! Glad you are enjoying the feature.

The fully expanded state is lost during rotation.

thanks for spotting, sounds like a technical one

Possible UX issue: When a site/tag is selected and I open the bottom sheet again and I click on the selected site/tag, I'd expect to de-select it.

That's interesting. I wonder is it the selected state. I actually tried a lot of different models for this but ended up trying not to add too much functionality too it. At the moment we're dealing with 'clearing the selection' with the x in the filter bar itself.

I think there's two ways we could adapt it:

  1. Treat it as a check box. Show a selected state. Tapping on the selected state unselects it. But in this case, I think the sheet should stay open. Currently the sheet closes on any selection, so its more like a quick switcher between sites.
  2. Removing the selected indicator could make it feel less like a checkbox/radio button and more like a shortcut.

Its not 100% clear to me that this is a big issue. But if I was to make a change I would actually do the second one, and not show a selected indicator. That might make it clearer how the thing is working, rather than changing how it works.

@develric
Copy link
Contributor Author

develric commented Feb 4, 2020

Hi @malinajirka and thanks for the usual insights you deliver in your review 🙇‍♂️!

  1. The fully expanded state is lost during rotation.

I should have fixed this one here 0efe735. Please note that the solution seems to work well but it's a bit naif 😜! Let me know wdyt.

  1. Possible UX issue

@osullivanchris , @malinajirka , it seems there are no strong opinions on this for now, can we maybe skip it for this PR? Let me know what do you think, thanks.

It seems we weren't on the same page when we discussed changing the method names

Thanks for this, I've already printed it and put on my wall for future reference 😜! (not kidding!)
I agree with your view and description of the pattern (that was also the one by @khaykov ); I think you got it right also that I was basically somewhere in between in being forced to manage the method naming more as commands (since a lot of logic is/was in the fragment) and more like events on which the VM can act and execute his logic after being notified.

I need to admit that trying to align the methods naming to the event convention even when they were currently commands is even worse, so I tried to revert the names I thought could make sense. Let me know wdyt, thanks Jirka 🙇‍♂️.

Having worked a bit now on the reader I think it would deserve a dedicated effort to refactor in the direction you highlighted since it is an important part of the app. Basically something similar to the project you were deeply involved about the EditPostActivity. Something to think of in next project/tasks proposal I guess.

@osullivanchris
Copy link

@osullivanchris , @malinajirka , it seems there are no strong opinions on this for now, can we maybe skip it for this PR? Let me know what do you think, thanks.

I think its ok to leave it for now. Hope you don't mind @malinajirka !

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Removing the selected indicator could make it feel less like a checkbox/radio button and more like a shortcut.

This sounds good to me ;)

it seems there are no strong opinions on this for now, can we maybe skip it for this PR? Let me know what do you think, thanks.

Yep I agree. Having said that, we might want to create a new GitHub issue so we don't forget about it. Wdyt?

I need to admit that trying to align the methods naming to the event convention even when they were currently commands is even worse, so I tried to revert the names I thought could make sense. Let me know wdyt, thanks Jirka 🙇‍♂️.

Yeah, I agree that making sure the naming corresponds to what is actually happening is more important than blindly following some naming conventions 👍

Having worked a bit now on the reader I think it would deserve a dedicated effort to refactor in the direction you highlighted since it is an important part of the app. Basically something similar to the project you were deeply involved about the EditPostActivity. Something to think of in next project/tasks proposal I guess.

Yeah, it deserves a huge refactor. Unfortunately, even the EditPostActivity refactoring is far from being done.

I should have fixed this one here 0efe735. Please note that the solution seems to work well but it's a bit naif 😜! Let me know wdyt.

Is there any reason why we need to set the default/initial state manually? Can't we simply remove
behavior.state = BottomSheetBehavior.STATE_HALF_EXPANDED
and let the system handle the configuration change?

@develric
Copy link
Contributor Author

develric commented Feb 4, 2020

Hi @malinajirka 😊, thanks for the quick follow up 🙇‍♂️!

Yep I agree. Having said that, we might want to create a new GitHub issue so we don't forget about it. Wdyt?

Good point 👍, created it here

Yeah, it deserves a huge refactor. Unfortunately, even the EditPostActivity refactoring is far from being done.

I'm sure EditPostActivity is going to be great 😎!

Is there any reason why we need to set the default/initial state manually? Can't we simply remove
behavior.state = BottomSheetBehavior.STATE_HALF_EXPANDED
and let the system handle the configuration change?

🤦‍♂️! You found a blind spot for me! I was so focusing on managing the state that didn't realize that just setting the peekHeight was enough! Thanks for highlighting that! 🙇‍♂️

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

You found a blind spot for me! I was so focusing on managing the state that didn't realize that just setting the peekHeight was enough! Thanks for highlighting that!

No worries ;). We've all been there so many times :D.

Thanks @develric for all the changes ;)! It looks good overall. I found one minor visual bug. I'm approving this PR, so feel free to merge it and fix the bug in a different PR or fix it here. It's up to you.

It seems the bottom sheet gets re-shown when the activity comes to foreground.

Steps

  1. Click on "Filter"
  2. Press Home button
  3. Bring the app back to foreground
  4. Notice the animation glitch
    bottom-sheet2

@develric
Copy link
Contributor Author

develric commented Feb 5, 2020

Hi @malinajirka 😊. Thanks for catching the above issue with the bottom sheet. If it's fine for you I have opened the following issue to track it and address in a separate PR.

While looking into it I noticed also a small thing in this PR I would like to address that is I think it's better to register/unregister the EventBus in the ReaderPostListViewModel only if INFORMATION_ARCHITECTURE_AVAILABLE is true. Going to open a dedicated quick PR for this one. Hope it's fine with you, thanks 🙇‍♂️.

@develric
Copy link
Contributor Author

develric commented Feb 5, 2020

As confirmed in slack with @malinajirka we are merging this one and moving the minor enhancement on the EventBus registration discussed here above in this dedicated PR .

@develric develric merged commit 8f6881f into develop Feb 5, 2020
@develric develric deleted the issue/10887-new-design-reader-bottom-sheet-step3 branch February 5, 2020 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IA Reader: filter bottom sheet sites/tags lists special cases.
5 participants