-
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
[Reader] Move announcement card inside the feeds #20872
[Reader] Move announcement card inside the feeds #20872
Conversation
Generated by 🚫 Danger |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr20872-911be72 | |
Commit | 911be72 | |
Direct Download | wordpress-prototype-build-pr20872-911be72.apk |
|
App Name | ![]() |
|
Flavor | Jalapeno | |
Build Type | Debug | |
Version | pr20872-911be72 | |
Commit | 911be72 | |
Direct Download | jetpack-prototype-build-pr20872-911be72.apk |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/tags-ia #20872 +/- ##
===================================================
- Coverage 41.02% 41.01% -0.01%
===================================================
Files 1508 1509 +1
Lines 69287 69289 +2
Branches 11428 11427 -1
===================================================
- Hits 28424 28422 -2
- Misses 38280 38291 +11
+ Partials 2583 2576 -7 ☔ View full report in Codecov by Sentry. |
71ebaa0
to
2375887
Compare
WordPress/src/main/java/org/wordpress/android/ui/reader/adapters/ReaderPostAdapter.java
Outdated
Show resolved
Hide resolved
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.
The code looks good. I only left a minor comment.
Also, i didn't test the gapMarkerPosition
logic, intended to display the gap in the feed. I left the app open and should be able to check it shortly.
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.
Hey thanks for the build. The spacing, colors, poisitioning feels pretty good. WDYT? Happy with it?
Hard to say for certain without seeing a build but the text sizes feel a bit small. Could you try bumping up the headings from 14 -> 16. And the excerpt from 12 to 14? Which I think would match the post excerpt. Can use whichever m3 naming matches those values.
lastly what do you think about the done button? any idea for another label? or should it be an x?
Thanks for the review @osullivanchris .
I think it's looking great! The good thing about the current implementation is that we should be able to use it again in the future if minor changes.
We plan to finalize all the Reader Tags + Announcement changes today (including merging to Should we go with the bigger font suggested here or keep it as is. Worst case we can update it during beta. I am in favor of using a bigger font since I agree the current one looks a bit small, but I will do it on a separate PR.
I like the done button and given the plan to merge the changes today, I think it's a bit too late to change it to an |
Hey @thomashorta on both counts happy to go with things as they are so we can merge today. If we change the font size it might need more tweaking with alignment and such. So let's try it out in a separate Pr and we can decide next week if it's better. I'll play around with it in Figma too. On the button that's cool I just wanted a second opinion. It feels more positive to tap this big button than an x which feels a bit more negative and would be a smaller tap target. Plus the animation you put in after I tap done feels pleasant |
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.
Thanks for the PR, @thomashorta!
It's working as expected and code LGTM. I've left a small suggestion.
...ess/src/main/java/org/wordpress/android/ui/reader/repository/ReaderAnnouncementRepository.kt
Outdated
Show resolved
Hide resolved
|
Fixes #20621
This improves the current behavior by moving the Reader Announcement card inside the feeds that show on the main Reader screen, instead of rendering it on the
ReaderFragment
app bar.I was not sure if the announcement should be also included in the new
Your Tags
feed so I didn't include it in this PR, but @develric and @osullivanchris let me know if it should be also added there.To Test:
This relies on a shared pref that is set when the card is dismissed, so to test multiple times you will need to clear app data (or if using an emulator push a shared pref file without the dismiss pref to override the app prefs file).
reader_announcement_card
feature config (in Debug settings)reader_announcement_card
feature config (in Debug settings)Your Tags
and non-main feeds (e.g.: tag preview, blog preview)Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):