-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Removing Feature Flag for IA reader feature. #11343
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great job @develric! The code changes look good. I've found several potential issues I'm not sure about. Some of them are probably not related this feature. Most of them are minor issues which might not be even worth fixing. For me the biggest issues is #1
. Wdyt?
- Swipe gestures are disabled - feels inconsistent with other parts of the app with tabs. I kept trying to swipe to change the tab but the app would instead keep opening post details.
- When I add a tag after a configuration change, the tag doesn't appear on the screen
- The items are not
horizontallyvertically centered which leads to misclicks
- The top bar collapses as I scroll down, however it expands itself after a configuration change
- When I unfollow a site, clicking on the follow icon doesn't do anything
- When I unfollow a site, the site disappears after configuration change
- Since we added "follow/unfollow" to "Followed Sites" section, we might want to consider adding it to "Sites you may like"?
If we decide to fix any of these issues, I think we might want to consider fixing them in separate PRs as it could easily get out of hand. Wdyt?
WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderPostListFragment.java
Show resolved
Hide resolved
Replying to some of the issues @malinajirka raised Issue 1 is known. Would be nice if it worked but we don't have it yet. Just wondering though - should we disable swipe to go to post detail? If swiping doesn't work its not ideal. But if it does something else unexpected, it could be worse. I wouldnt treat it as a blocker though, an improvement. Numbers 2-6 seem like technical issues. Good spot on number 3, that's cool. On number 7, sure, I didn't think of that. Seems like a no-risk move. Again, it can be captured as an improvement task, definitely not a blocker. Excited to see this coming together :) |
Hey @malinajirka 👋 I can't reproduce Issue 5. Do you mind sharing which device/emulator and Android version you're using? |
Hey @malinajirka 👋! Thanks for the thorough review, much appreciated 🙇♂️! I try to follow up to the items below; in my understanding they should be not blocking so I'm addressing some of them now and creating tracking issue for others. Let me know wdyt 😊:
Note: I report also here something we discussed in a quick chat we had on slack regarding a possible issue with the following call path in
it can happen that the onRecallSelection (who calls requireActivity) could be called when the fragment is not added so resulting in a crash. Created a small PR for this here #11360 targeting this PR branch |
Preserve toolbar collapsing state in reader.
…issue-when-fragment-not-attached Ensuring the fragment isAdded before to continue with the AsyncTask.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you all for the replies ;)!
Thanks @develric for creating the issues and even fixing some of them ;). Since none of the issues seems vital, I'm approving this PR. I can't merge it right now because of an ongoing CI checks. Feel free to merge it when the CI is done.
This PR has been created to move the IA reader modifications from behind the
INFORMATION_ARCHITECTURE_AVAILABLE
feature flag and release it into develop.cc @osullivanchris , @khaykov , @renanferrari
Main items introduced
Main linked PRs (and related test steps)
Smoke test rules of thumbs
If you want/can give this PR a smoke test, use both wp.com and self-hosted sites and here below some of the possible steps for smoke testing but feel free to add 😊 :
Note
While testing this PR we found a small issue with the self-hosted empty view for cta.
steps to reproduce
FOLLOWING
and no subfilter is selectedWe fixed it in 78c4b67
PR submission checklist:
RELEASE-NOTES.txt
if necessary.