Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Feature] Reader Improvements #19267

Merged
merged 233 commits into from
Oct 16, 2023
Merged

[Feature] Reader Improvements #19267

merged 233 commits into from
Oct 16, 2023

Conversation

RenanLukas
Copy link
Contributor

@RenanLukas RenanLukas commented Sep 26, 2023 •
edited
Loading

Feature branch of Reader Improvements (ref: pcdRpT-3Eb-p2)

  • Tag and Site feed header
  • Post details header
  • Post feed cards
  • “You might like” and “Sites to follow” cards
  • Follow/Unfollow buttons

@RenanLukas
Implement reader improvements feature flag
4766c75
@RenanLukas
Add noto serif font to project
887674e
@RenanLukas
Update site header view design
ec4d348
@RenanLukas
Merge pull request #19080 from wordpress-mobile/issue/19079-reader-im…
149cefa 
…provements-feature-flag

[Reader Improvements] Implement Reader Improvements feature flag
@RenanLukas
Implement empty toolbar title for site post list in Reader
ebc2f3d
@RenanLukas
Fix reader site header avatar padding
31a5b0b
@RenanLukas
Add reader improvements feature flag to site header layout
1c1a2c6
@RenanLukas
Fix reader post list toolbar scroll flags
dc32562
@RenanLukas
Fix reader site header icon border size
6ac72a9
@RenanLukas
Fix reader site header description emphasis
2762453
@RenanLukas
Update reader post list activity back navigation icon from X to arrow
ede0386
@RenanLukas
Remove share button from reader site header toolbar
48d86ed
@RenanLukas
Revert reader site header description emphasis to medium
7a2902a
@RenanLukas
Hide reader site header icon if image URL is empty
eac2d1c
@RenanLukas
Fix reader site header followers string
6c1b531
@RenanLukas
Fix lint: remove unused menu file share.xml
a7feef3
@RenanLukas
Rename loadBlavatarImage -> showBlavatarImage
a789f47
@RenanLukas
Update reader site header new design site icon placeholder
f189899
@RenanLukas
Fix reader site header margins
a91261d
@RenanLukas
Extract Reader header follow container to new file
edf35c4
@RenanLukas
Extract reader header title style
a18a4b6
@RenanLukas
Update tag header view design
9890449
@RenanLukas
Add reader improvements feature flag to tag header layout
d003bda
@RenanLukas
Hide toolbar title for reader tag posts list
8fef5a3
@RenanLukas
Fix DI inject call order
b9dcc6a
@RenanLukas
Fix reader tag posts list header margins
c3142a5
@RenanLukas
Hide reader header follow count text by default
75f1f5e
@RenanLukas
Update reader site header with extracted style
33d83a4
@RenanLukas
Replace follow button and followers label with extracted follow conta…
3189274 
…iner in reader site header new design
@RenanLukas
Hide followers count for reader tag header new design
b8b9639
@RenanLukas
Copy link
Contributor Author

Thank you for taking the time, @tiagomar 👍

Merge pull request #19354 from wordpress-mobile/issue/fix-follow-butt…
3744203 
…on-manage-topics-sites-screen

[Reader Improvements] Fix follow buton size on "Manage Topics & Sites" screen
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 12, 2023 •
edited
Loading

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

Thomas Horta and others added 12 commits October 12, 2023 11:14
Merge pull request #19352 from wordpress-mobile/issue/19337-reader-po…
0a486f6 
…st-card-minor-tweaks

[Reader Improvements] Post card minor tweaks
@RenanLukas
Fix follow/unfollow button responsiveness on reader site header
5f06569
Add isFollowEnabled field to Recommended Blog UI state
fa178f9
Add isChangeFinal field to FollowStatusChanged
28b856f 
This new field keeps track if the current status change is final or not, meaning
that it was committed and succeeded in the backend or was just an initial status
change due to the action being STARTED.
Update Follow button status for recommended blog Discover cards
5943987 
Observe the `followStatusUpdate` LiveData to instantly change the state of the
follow button for the recommended blogs card in the Discover feed. It also sets
the button as enabled or disabled based on the status updated.

Currently there's a small issue that makes the button be enabled even before the
final change in state for `followStatusUpdate` because the Discover feed is also
updated by another data source (`readerDiscoverDataProvider.discoverFeed`) which
is directly hooked to the `ReaderPostTableActionEnded` that is emitted by the
Event Bus when there are changes in the underlying table.

This issue is fairly small since the worst thing that can happen is the ripple
animation showing up but there are other safeguard in place to avoid making
duplicate request so the overall state does not get insconsistent.
Use isChangeFinal in PostDetail header to enable/disable follow button
207bb26 
Use the new `isChangeFinal` field to properly update the enabled/disabled status
of the follow button in the Post Detail header.
Fix broken unit tests
17a7920
Refactor ReaderDiscoverViewModel init
d85453f
Default ReaderImprovementsFeatureConfig (reader_improvements) to ON
bc2a239
Update bookmark UI test for reader improvements
03be7cf
Merge pull request #19363 from wordpress-mobile/issue/19307-fix-follo…
c2361c8 
…w-unfollow-button-responsiveness

[Reader Improvements] Fix follow unfollow button responsiveness
Merge pull request #19371 from wordpress-mobile/issue/19370-reader-im…
a1a5b80 
…provements-enable-ff

[Reader Improvements] Enable ReaderImprovementsFeatureConfig by default
@RenanLukas RenanLukas marked this pull request as ready for review October 13, 2023 21:25
@RenanLukas RenanLukas requested a review from a team as a code owner October 13, 2023 21:25
@develric develric added Tooling and removed Tooling labels Oct 14, 2023
@ravishanker ravishanker added Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging and removed Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging labels Oct 15, 2023
jostnes
jostnes approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@jostnes jostnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran ReaderTests locally and looks good, the previously failing test e2eBookmarkPost now works without issue. The changes look good too. Thanks for fixing the failing test! :shipit:

@ravishanker ravishanker removed the Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging label Oct 16, 2023
@ravishanker ravishanker merged commit 7969cf0 into trunk Oct 16, 2023
2 checks passed
@ravishanker ravishanker deleted the feature/reader-improvements branch October 16, 2023 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiagomar tiagomar tiagomar approved these changes

@jostnes jostnes jostnes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
23.5
Development

Successfully merging this pull request may close these issues.
6 participants
@RenanLukas @wpmobilebot @jostnes @tiagomar @ravishanker @develric