-
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] Implement tags feed UI #20686
[Reader] Implement tags feed UI #20686
Conversation
…into issue/20623-implement-horizontal-posts-list-ui-component
…into issue/20623-implement-horizontal-posts-list-ui-component
…into issue/20623-implement-horizontal-posts-list-ui-component
…into issue/20623-implement-horizontal-posts-list-ui-component
…into issue/20623-implement-horizontal-posts-list-ui-component
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/tags-ia #20686 +/- ##
===================================================
+ Coverage 40.15% 40.33% +0.17%
===================================================
Files 1465 1474 +9
Lines 67522 67878 +356
Branches 11183 11225 +42
===================================================
+ Hits 27114 27377 +263
- Misses 37959 38034 +75
- Partials 2449 2467 +18 ☔ View full report in Codecov by Sentry. |
<!-- Reader tags feed --> | ||
<string name="reader_tags_feed_see_more_from_tag">More from %s</string> | ||
<string name="reader_tags_feed_error_title">No posts found for %s</string> | ||
<string name="reader_tags_feed_error_description">This tag might not have any posts, or there was no internet connection.</string> |
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.
This error message is too generic, as it combines "no posts found in tag" (which is a valid condition) with a generic error. Can we make it clearer whether it's a real error or simply no posts found?
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.
Makes sense. There is at least one more scenario: when there was an error with the request. Should we have three different messages (one for when posts list returns empty, one for no internet connection and another for a failed request)? cc @osullivanchris
Also, if the post list returns empty from the back-end, does it make sense to provide a retry button? In theory it's going to return an empty post list again.
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.
I think two errors is enough as it might be difficult and not meaningful to distinguish between when the user had no internet connection, and when we failed to fulfil the request.
Something like:
- Posts failed to load: we couldn't load posts from this tag right now
- No posts for this tag: we couldn't find any posts tagged TagName right now
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.
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.
Posts failed to load: we couldn't load posts from this tag right now
If we already have some posts locally under a certain tag, what do you think about displaying them instead of showing an error? This means the Posts failed to load
message only appears when we have 0 posts under for this tag AND the request failed.
Quality Gate failedFailed conditions |
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.
Nice work! LGTM
Fixes #20623
Light theme
Screen.Recording.2024-04-19.at.9.09.17.PM.mov
Dark theme
Screen.Recording.2024-04-19.at.9.09.45.PM.mov
To Test:
The UI component is not being used yet, so in order to test it you can run
ReaderTagsFeedLoaded
andReaderTagsFeedLoading
previews and compare them with Figma specs.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):