nowinandroid icon indicating copy to clipboard operation
nowinandroid copied to clipboard

navigating to browse the topic on NiaTopicTag click

Open yveskalume opened this issue 2 years ago • 11 comments

yveskalume avatar Nov 17 '22 21:11 yveskalume

LGTM! Let me check with our design team about this change before merging.

calren avatar Nov 17 '22 21:11 calren

Hey @yveskalume, can I ask you to resolve the conflicts in the meantime? Thanks!

mmoczkowski avatar Nov 22 '22 14:11 mmoczkowski

Done, but there are some files included in the diff that's not part of this PR because of the merge with main branch.

yveskalume avatar Nov 22 '22 15:11 yveskalume

Thanks for submitting this. To remove the extra commits, please do:

git rebase main then force push git push -f onto your branch.

dturner avatar Nov 22 '22 21:11 dturner

Thanks for submitting this. To remove the extra commits, please do:

git rebase main then force push git 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)

yveskalume avatar Nov 23 '22 05:11 yveskalume

Feel free to do it here!

calren avatar Nov 23 '22 05:11 calren

Done ✅

yveskalume avatar Nov 23 '22 12:11 yveskalume

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.

dturner avatar Nov 24 '22 11:11 dturner

Can I just add tests to this PR and then work on the other issue ?

yveskalume avatar Nov 24 '22 11:11 yveskalume

Yes, fine to just add tests in this PR.

dturner avatar Nov 24 '22 17:11 dturner

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

Screenshot 2022-11-26 at 10 33 54

yveskalume avatar Nov 26 '22 08:11 yveskalume

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.

dturner avatar Jan 13 '23 12:01 dturner