nowinandroid
nowinandroid copied to clipboard
navigating to browse the topic on NiaTopicTag click
LGTM! Let me check with our design team about this change before merging.
Hey @yveskalume, can I ask you to resolve the conflicts in the meantime? Thanks!
Done, but there are some files included in the diff that's not part of this PR because of the merge with main branch.
Thanks for submitting this. To remove the extra commits, please do:
git rebase main
then force push git push -f
onto your branch.
Thanks for submitting this. To remove the extra commits, please do:
git rebase main
then force pushgit push -f
onto your branch.
Okay, can I directly do that here or should I raise a new PR (it'll be the third one)
Feel free to do it here!
Done ✅
This is looking great, thanks. A few comments/suggestions...
Please add UI tests to NavigationTest
to verify that:
- When tapping on a topic chip the user is taken to the correct Topic detail screen.
- When going back from the Topic detail screen the scroll position on the ForYou screen is remembered (currently it isn't)
There's also a (kind of) bug. When the user taps on a topic they are taken to the Topic detail screen where they see a list of news resources, all of which have that topic in the list of topic chips. Tapping on that topic takes them to the same Topic detail screen, which pushes a duplicate instance of the same screen onto the back stack. Ideally tapping on a topic chip when you are already on the Topic detail screen for that topic should do nothing.
You can either fix this in this PR, or submit the PR and file a bug for the above.
Can I just add tests to this PR and then work on the other issue ?
Yes, fine to just add tests in this PR.
Hi @dturner I would like to have some help, I don't have too much experience with compose ui testing and I would love to have some advice on how to proceed. At the moment, my test is only failing. Thanks
Sorry for the delay in responding here. After some internal discussions we've decided that the topic chip interaction shouldn't have a drop down menu for following/unfollowing. This is because the act of following/unfollowing a topic will cause the news feed to completely refresh, and the user may lose their place in the feed. This is not a good user experience, especially in accessibility scenarios. Also, what if the news resource card no longer appears in the list? The drop down has now been orphaned.
For this reason we're going to abandon this in favour of a simple interaction which when tapping on topic chip will take you to the topic detail screen.
Thanks for your efforts.