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

Add follow/unfollow button to the Followed Sites tab on Reader's management section #11219

Merged
merged 5 commits into from
Feb 12, 2020

Conversation

renanferrari
Copy link
Member

Fixes #11167

This PR adds a follow/unfollow button to the Followed Sites tab on Reader's management section. Some considerations that probably need to be reviewed by a designer:

  1. Right now, when the user taps on the button, the site gets followed/unfollowed in the background and the icon animates between both states.
  2. The list is not automatically updated to reflect the changes, as that would simply cause the site to immediatelly disappear if the user unfollowed it. Instead, we leave the site in place while the screen is open, in case the user wants to easily revert the change.
  3. No further visual feedback is given to user, unless an error occurs, which then shows a toast with the error message while the button animation is reverted.

To test

For each of the tests below, you first need to make sure you're using the build variant thas has enabled IA flags.

1. Unfollow

  1. Open the Reader.
  2. Select the Following tab.
  3. Tap on the cog icon on the right side, below the tab bar.
  4. Select the Followed Sites tab.
  5. Notice the follow/unfollow icon besides each item in the list.
  6. Tap on the follow/unfollow icon.
  7. Notice the animation between follow/unfollow states.
  8. Close the screen.
  9. Verify that the site is no longer being followed.

2. Undo Unfollow

  1. Open the Reader.
  2. Select the Following tab.
  3. Tap on the cog icon on the right side, below the tab bar.
  4. Select the Followed Sites tab.
  5. Notice the follow/unfollow icon besides each item in the list.
  6. Tap on the follow/unfollow icon.
  7. Notice the animation between the follow/unfollow states.
  8. Tap on the follow/unfollow icon again.
  9. Notice the animation between the follow/unfollow states.
  10. Close the screen.
  11. Verify that the site is still being followed.

PR submission checklist:

  • 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.

@renanferrari renanferrari added [Type] Enhancement Reader [Status] Needs Design Review A designer needs to sign off on the implemented design. IA labels Feb 1, 2020
@renanferrari renanferrari added this to the 14.2 milestone Feb 1, 2020
@renanferrari renanferrari requested a review from develric February 1, 2020 04:20
@renanferrari renanferrari self-assigned this Feb 1, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 1, 2020

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

@renanferrari renanferrari removed their assignment Feb 1, 2020
@osullivanchris
Copy link

Hi! I downloaded the APK above but for some reason I am not seeing the correct reader version. Maybe because it is dependent on the other PRs being merged. But reviewing based on what you have described:

Right now, when the user taps on the button, the site gets followed/unfollowed in the background and the icon animates between both states.

Sounds good. I'm assuming its the same animation we use elsewhere for this state change. (e.g. when you follow/unfollow from a single site view

The list is not automatically updated to reflect the changes, as that would simply cause the site to immediatelly disappear if the user unfollowed it. Instead, we leave the site in place while the screen is open, in case the user wants to easily revert the change.

I agree with your decision here. I have encountered this issue before and what you described is common practice. I would just like to clarify that if I unfollow a site, then leave this page, and return again...will it then be gone for the list? Or what is the mechanism that determines when the item does disappear?

No further visual feedback is given to user, unless an error occurs, which then shows a toast with the error message while the button animation is reverted.

I think this is ok as we do not display any toast in the existing single site case

Thanks for tagging for design review. Feel free to @ me on any of the iA stuff as well. Thanks for the helpful description too. And if APKs or screenshots are available to take a look myself even better!

@renanferrari
Copy link
Member Author

I would just like to clarify that if I unfollow a site, then leave this page, and return again...will it then be gone for the list?

Yes, that's correct.

And if APKs or screenshots are available to take a look myself even better!

I'm not familiar with the system that generates the APK, but I'm guessing it doesn't generate the one with the iA flags. I'll try to record the screen and share it.

@renanferrari renanferrari added [Status] Needs Code Review and removed [Status] Needs Design Review A designer needs to sign off on the implemented design. labels Feb 7, 2020
@renanferrari
Copy link
Member Author

Thank you @osullivanchris for confirming the changes are ok in Slack. Moving on to code review now.

@develric
Copy link
Contributor

Hey @renanferrari 👋! I wen into this one and looking good overall 👍!

I found an issue with the rss feeds. From the FOLLOWED SITES page we can decide to follow also an RSS feed (like this one for example https://www.ansa.it/sito/ansait_rss.xml). When trying to unfollow an RSS feed the action does not succeed. I think you could have a look in other places in the code base (like ReaderSiteHeaderView). It seems we are using dedicated methods for Feeds vs Blogs.

Let me know wdyt 😊.

@jkmassel
Copy link
Contributor

We're freezing 14.2 today, so this PR is being bumped to 14.3. If you'd like it to ship with 14.2, please merge it into release/14.2 and ping me – I'll be happy to cut a new beta release.

@jkmassel jkmassel modified the milestones: 14.2, 14.3 Feb 10, 2020
@renanferrari
Copy link
Member Author

@develric Thanks for pointing that out, I was not aware of this use case. I just pushed some changes that should fix this.

Copy link
Contributor

@develric develric left a comment

Choose a reason for hiding this comment

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

Hey @renanferrari thanks for the update! I checked it and works as expected! Good job 👍

Just as a side note, at least on my emulator, the follow/unfollow button stays disabled for a couple of seconds before I can switch the state again, but looking at it, it seems that is the time during which we wait for the API response. Also I get the same behavior with follow/unfollow buttons in other parts of the app, so I guess it is expected. LGTM :shipit:

@develric develric merged commit e0a5dde into develop Feb 12, 2020
@develric develric deleted the issue/11167-reader-followed-sites-button branch February 12, 2020 22:04
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.

Reader followed sites/tags management section small tweak
4 participants