[WIP] HACK Week: Reader detail customization #22851

dvdchr · 2024-03-18T19:28:00Z

Internal refs: zQnohyMpLzBzQ5jzMMKni3-fi-3900_17941, p1710343618122149-slack-C06PGTY5PKK

Warning

Some of the display settings are not fully applied yet, namely:

The comments section
Related posts
Toolbar
Navigation bar items

Since the feature is wrapped under a feature flag, I'll plan to address them separately via small-sized PRs.

This covers the proof-of-concept (WIP) of applying custom display settings to the Reader Post screen. The core idea is around the object ReaderDisplaySetting, which encapsulates the preferred color, font, and size.

The main part of customization, the post, is relatively easy. We just need to expose the display setting to the ReaderWebView, and allow the colors to be injected to the CSS styles.
The tricky/arduous part is actually the view components around the web view; like the header view, the comment table, the related posts section, and the navigation bar. Some of them have hardcoded background colors, so I needed to ensure that they are transparent.
Some definitions (such as secondary text color and font size scaling values) were missing, so I defined the values myself for now. Other details were also replaced with something "easier" to get it done faster—for example, I haven't imported the customization icon yet, so I used one from SF Symbols instead. Slider is another example; I used the "stock" Slider appearance for now, as the one from Figma requires custom implementation.

To figure out next

Consider if the way ReaderDisplaySetting is used and/or passed around is the "right" approach.
Consider somehow merging ReaderDisplaySetting to the design system or standard colors.
How to make the views react when the display setting is updated? In the current PR, I had to painstakingly update each component so that the colors are injectable.
- With the display settings integrated to the standard colors, views could automatically use the selected colors.
- But the problem is, how to make the views react when the selected colors are changed?
Figure out some other color components for each scheme, such as secondary text colors, secondary background colors, border colors, etc.
Review if the font generation method is correct, considering the customization variables. Ensure that they respect the preferred content size category, etc.

To test

Note that the functionality to save the updated preferences hasn't been added yet. For now, you can test bringing up the customization interface and play around with the selections by:

Going to any Reader post
Tap the customization icon (next to the "Open in Safari" navigation bar button)

Regression Notes

Potential unintended areas of impact
Should be none. Feature is hidden behind a feature flag.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manually tested the changes.
What automated tests I added (or what prevented me from doing so)
N/A.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

dangermattic · 2024-03-18T19:28:53Z

	1 Warning
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2024-03-18T19:36:31Z

📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.

	App Name	WordPress Alpha
	Configuration	Release-Alpha
	Build Number	pr22851-5eaac2d
	Version	24.5
	Bundle ID	org.wordpress.alpha
	Commit	`5eaac2d`
	App Center Build	WPiOS - One-Offs #9316

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

wpmobilebot · 2024-03-18T19:36:51Z

📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.

	App Name	Jetpack Alpha
	Configuration	Release-Alpha
	Build Number	pr22851-5eaac2d
	Version	24.5
	Bundle ID	com.jetpack.alpha
	Commit	`5eaac2d`
	App Center Build	jetpack-installable-builds #8360

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

…ySetting

The priority is still given to the `ButtonColor` object that's passed through the parameter. The `ReaderDisplaySetting` only takes precedence when there are no `ButtonColor` objects being passed. The `@State` wrapper is removed from the `color` property since it seems to be the cause of the button not updating colors after the display setting is updated.

wargcm

On an iPhone SE with default font settings, there are some display issues with the customization sheet. It appears too big for the screen:

Portrait	Landscape

I found some display issues while changing the font size without navigating away from the screen:

resizing.mp4

I'm guessing this may be part of the toolbar work, but figured I'd mention it just in case. I noticed the close (X) button is a bit difficult to see with some configurations.

Light mode + Evening:

Light mode + OLED:

Given that all these are pretty minor and it being a work in progress, I'm good approving it. Overall, nice work!

wargcm · 2024-03-28T21:13:32Z

WordPress/Classes/ViewRelated/Comments/ContentRenderer/WebCommentContentRenderer.swift

-        if let contentCache = commentContentCache, contentCache == comment.content {
-            return webView
+        if #available(iOS 16.4, *) {
+            webView.isInspectable = true


I'm guessing this was mostly for debugging. Should this be removed?

Hmm, I was thinking of leaving it in. I notice other web view subclasses/containers also have the isInspectable property enabled, so I think it's fine to ship this into production.

I'll leave it in for this PR, but we can continue to discuss, and I can remove it later during beta if we feel this is unsafe.

WordPress/Classes/ViewRelated/Reader/Detail/Views/ReaderDetailNewHeaderView.swift

WordPress/Classes/ViewRelated/Reader/Theme/ReaderDisplaySetting.swift

WordPress/Classes/ViewRelated/Reader/Theme/ReaderDisplaySettingViewController.swift

osullivanchris

testing the build and looking good so far. Approving per request but noting a couple things (that maybe fixed already elsewhere, thats why I'm not blocking this). When I made the text size largest:

the layout for the tags broke. Only seemed to happen first time, not the second time.
the article got cut off when I went back. Its like it remembered the initial article length in size and then didn't realise the text was now longer

dvdchr · 2024-03-29T15:32:42Z

Thanks for the review @wargcm & @osullivanchris!

Since this is still hidden behind a feature flag, I will aim to merge this today and address the remaining issues through smaller PRs so it's manageable for us and so that the PR is not too "different" from when you reviewed it last 😅 I'll open an issue after this to log the issues so that we can keep track of it. 🙂

dvdchr added Reader [Status] DO NOT MERGE labels Mar 18, 2024

dvdchr marked this pull request as draft March 19, 2024 15:32

thomashorta mentioned this pull request Mar 19, 2024

HACK Week: Reader detail customization wordpress-mobile/WordPress-Android#20506

Merged

13 tasks

dvdchr requested review from wargcm and osullivanchris March 20, 2024 12:36

dvdchr self-assigned this Mar 20, 2024

dvdchr force-pushed the hackweek/reader-post-customization branch from 76dcbfe to d684c03 Compare March 25, 2024 15:50

dvdchr added 10 commits March 27, 2024 19:40

Add a model structure for Reader customization

e1f7d11

Add access to the display setting object from detail vc

5d42c30

Set web-rendered comment background to transparent

7167d95

Keep isOpaque to false to maintain transparency

7e6c52b

Set background colors to transparent in Reader comment table

08e1224

Configure background colors in post detail to respect selected displa…

54708af

…ySetting

Set topics collection view background to transparent

e5bf85c

Inject display setting colors to the header view

a1f94d2

Add display setting selection sheet (wip)

56c7be3

Add barButtonItem to change display settings

51da0d3

dvdchr force-pushed the hackweek/reader-post-customization branch from d684c03 to 51da0d3 Compare March 27, 2024 12:40

dvdchr added 8 commits March 28, 2024 21:20

Add feature flag

4c977b3

Update color theme names

a79ca3c

Disable olde and rock font options

88b04ae

Refactor display setting view

f318fba

Add a completion block parameter

c7a6df6

Add a bit of space to the bottom of Done button

cb31caf

Initialize slider value based on displaySetting size value

90168a8

dvdchr added 2 commits March 29, 2024 03:39

Apply font and custom sizing to Reader web view

c90ec54

Apply display setting font and size to the header view

df2087a

dvdchr added this to the 24.6 milestone Mar 28, 2024

dvdchr added Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. [Type] Enhancement and removed [Status] DO NOT MERGE labels Mar 28, 2024

dvdchr marked this pull request as ready for review March 28, 2024 20:47

wargcm approved these changes Mar 28, 2024

View reviewed changes

osullivanchris approved these changes Mar 29, 2024

View reviewed changes

dvdchr added 7 commits March 29, 2024 21:54

Properly retain displaySetting parameter

5c257ec

Wrap the tags view in the preview in a scrollview

2c60ba0

Use the primaryTextColor variable

6356219

Convert display setting color labels to NSLocalizedString

62d83fb

Remove unused font options

486d4dc

Clean up display setting Size enum

fa68a94

Use localized string for Slider label and update localized string keys

f9cf690

dvdchr added 2 commits March 29, 2024 22:37

Fix typo in slider configuration after updating Size enum

6181301

Fix font weight not being applied correctly for the display setting

30551ac

dvdchr force-pushed the hackweek/reader-post-customization branch from 08e43bd to 12e74be Compare March 29, 2024 16:40

Rename default to standard to fix CI issues

b13bc83

dvdchr force-pushed the hackweek/reader-post-customization branch from 12e74be to b13bc83 Compare March 29, 2024 16:44

Add fallback value to address warning

5eaac2d

dvdchr merged commit f6f8890 into trunk Mar 29, 2024
24 checks passed

dvdchr deleted the hackweek/reader-post-customization branch March 29, 2024 19:37

This was referenced Mar 30, 2024

Reader Customization #22925

Open

Reading Preferences: Fix sizing issues for post detail and web-based comments #22939

Merged

Reading Preferences: Fix customization sheet #22967

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] HACK Week: Reader detail customization #22851

[WIP] HACK Week: Reader detail customization #22851

dvdchr commented Mar 18, 2024 •

edited

Todo

dangermattic commented Mar 18, 2024 •

edited

wpmobilebot commented Mar 18, 2024 •

edited

wpmobilebot commented Mar 18, 2024 •

edited

wargcm left a comment

wargcm Mar 28, 2024

dvdchr Mar 29, 2024

osullivanchris left a comment

dvdchr commented Mar 29, 2024 •

edited

[WIP] HACK Week: Reader detail customization #22851

[WIP] HACK Week: Reader detail customization #22851

Conversation

dvdchr commented Mar 18, 2024 • edited

Todo

To figure out next

To test

Regression Notes

dangermattic commented Mar 18, 2024 • edited

wpmobilebot commented Mar 18, 2024 • edited

wpmobilebot commented Mar 18, 2024 • edited

wargcm left a comment

Choose a reason for hiding this comment

wargcm Mar 28, 2024

Choose a reason for hiding this comment

dvdchr Mar 29, 2024

Choose a reason for hiding this comment

osullivanchris left a comment

Choose a reason for hiding this comment

dvdchr commented Mar 29, 2024 • edited

dvdchr commented Mar 18, 2024 •

edited

dangermattic commented Mar 18, 2024 •

edited

wpmobilebot commented Mar 18, 2024 •

edited

wpmobilebot commented Mar 18, 2024 •

edited

dvdchr commented Mar 29, 2024 •

edited