[RNMobile] Cover block: Support isDark attribute
#40691
Conversation
fluiddot
added
Mobile App - i.e. Android or iOS
|
Size Change: +6 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
@fluiddot I'm unable to get the mobile test steps to pass. I've tried on iPhone 11/ iOS 14.8 and Pixel 6/ Android 12. |
@jhnstn Good catch! Looks like this is expected for dark themes, as it's outlined in this PR's description:
Could you try using a different theme and check if the issue persists? Thanks 🙇 ! |
I switched to "Quadrat Black" and the text is still not changing: RPReplay_Final1651175218.MP4Also forgot to note previously that with "twenty twenty two", the cover text was changing colors on production web. |
That's interesting, looks like depending on the theme these changes behave differently 🤔. I'm afraid this is related to how we calculate the final color to be applied in the gutenberg/packages/block-library/src/cover/edit.native.js Lines 271 to 273 in d680915 gutenberg/packages/rich-text/src/component/index.native.js Lines 1268 to 1271 in 4ccdee3 gutenberg/packages/rich-text/src/component/index.native.js Lines 1204 to 1209 in 4ccdee3 When using Using However, when using a theme that defines global styles like Using NOTE: An easy way to identify themes that define global styles is by checking if the menu item
As far as I checked, this behavior differs from the web version where the global styles have less preference than the parent styles, in this case, defined by the Cover block:
@jhnstn Not sure if we'd like to address the style preference issue in this PR, as it might be a bit complex, so I'm wondering if we could tackle it in a different PR and create an issue as a follow-up for that, wdyt? |
💯 I wouldn't want to attempt those changes here. I'll test again with the a theme that lets me try out these specific changes. |
Agreed, I'll create a new issue as a follow-up on this 👍. |
There was a problem hiding this comment.
@fluiddot I was able to test out the changes on mobile and web. Everything worked as expected. I was unable to test the starter page templates however. I was running into issues creating a new site on mobile with my Automattic account. Overall the changes look good, nice work!
|
Thanks for the review @jhnstn 🙇 !
It would be great to test the starter page templates as it's the issue that originated these changes (reference). Regarding the issues creating a new site, I'm wondering if they are related to actual bugs, could you share what you encountered? Thanks! @SiobhyB as I also assigned you as a reviewer, I'm wondering if you could take a quick look at the "Check starter page templates" test case. Thanks for the help 🙇 ! |
# Conflicts: # packages/react-native-editor/CHANGELOG.md
FYI I've just created this issue as a follow-up. |
There was a problem hiding this comment.
Interestingly, I wasn't able to reproduce the original issue on an iOS simulator. I noticed Tanner mentioned there were times when the issue wasn't reproducible for him at wordpress-mobile/WordPress-iOS#18024 (comment), too. 🤔
I was able to reproduce the original issue on both a physical Android and iOS device, however.
I wanted to double-check that everything worked as expected on my physical devices, given my difficulty reproducing on an emulator, so created installable builds at wordpress-mobile/WordPress-Android#16466 and wordpress-mobile/WordPress-iOS#18495.
Everything seemed good on my Android device, but I'm still seeing the issue with the text appearing black in the preview on my iOS device. I see the issue when previewing pages for existing sites and after creating a brand new site from scratch.
I noticed the WordPress.com toolbar in the top of the second Android screenshot, so not sure if that may be relevant to the issue appearing fixed there.
@fluiddot, let me know if I'm perhaps missing something here or if I can help with any further testing.
iOS
trunk on physical iPhone XR |
Changes on physical iPhone XR |
|---|---|
 |
 |
Android
trunk on physical Pixel 6 |
Changes on physical Pixel 6 |
|---|---|
 |
 |
That's interesting, in my case, I was able to reproduce it on both simulators and physical devices 🤔.
👍
Great, thanks for generating the installable builds 🙇.
Oh, when I tested locally using my devices I checked that the fix worked on both platforms 🤔. I noticed that the WP-iOS PR is pointing to the latest commit of wordpress-mobile/gutenberg-mobile#4808, but the JS bundle for iOS wasn't updated. I think that explains why you can reproduce it on iOS and not on Android. I'm going to update the bundle and update both GB-mobile and WP-iOS PRs so the iOS installable build reflects the changes from this PR 💨.
I'd say that is not related to these changes, or at least it's not expected. |
@SiobhyB As I commented here, I'm going to update the JS bundle in the GB-mobile PR and generate a new iOS installable build. I'll let you know when it's ready to double-check that the issue was caused by the out-to-date bundle and not the changes 🤞 . |
The issues were unrelated to the app for sure. There might have been an issue with the WP.com API but I'm fairly certain the issue is with my Automattic account and the WP.com store code. I can spin up the installable builds and try again with my test accounts. |
Great, thanks for clarifying it 👍.
That would be great, thanks @jhnstn ! I already triggered a new build, I'll post a comment in the PR once they're ready to use. |
There was a problem hiding this comment.
@fluiddot, aha, you're right, sorry for not spotting the out-of-date bundle! I saw the new build you generated is available now and confirmed the issue is fixed there:
All looks good from my side, thanks for fixing this 🙇♀️
|
Thanks @SiobhyB for checking it 🙇 ! I was about to comment that the installable build is ready but you beat me 😅: |
# Conflicts: # packages/react-native-editor/CHANGELOG.md
|
@jhnstn @SiobhyB I'd also appreciate it if you could also review the GB-mobile PR. Note that the |
What?
Add support for the
isDarkattribute in the native version of the editor. This attribute improves the color contrast between the background and text, by changing the text color to white/black depending on the overlay color and opacity. As a side note, this only applies to text elements that don't have a color set.Why?
This PR aims to fix wordpress-mobile/WordPress-iOS#18024 and add support for the
isDarkattribute introduced in #33541.How?
Extract
useCoverIsDarkhook to a fileSince this hook holds logic that can be used on both platforms, I decided to extract it to a new file.
I'd like to note that In the web version, when the opacity of the block is lower than or equal to 50%, it calculates the average color of the content (i.e. image or video) and uses it for determining the
isDarkvalue. However, the library for that purpose only works on web, so until we provide a workaround for this calculation in native, this case won't be handled.NOTE: This might introduce discrepancies when displaying a Cover block, especially on the native version when the
isDarkattribute changes by modifying the opacity.web-cover-block-dark-mode.mp4
native-cover-block-dark-mode.mp4
Handle
isDarkattribute in nativeSimilar to how it's handled in the web version, the hook is called and uses the value returned to set dark or light color to the content via the
childrenStylesattribute.Additionally, these changes cover the case spotted in wordpress-mobile/WordPress-iOS#18024 (comment) where the
is-lightclass is set when upgrading the block from an older version. The workaround for this is basically to ensure that, ifisDarkattribute istrue, theis-lightclass is removed.Testing Instructions
Check light/dark modes when
isDarkattribute changesNOTE: As outlined in #40691 (comment), this functionality is not working when using themes that provide global styles. In order to test the changes, consider using a theme without global styles like
Twenty Twenty-OneorAlves.false.Check potential regressions in the web version
false.NOTE: There's a known bug related to setting the black color even for images that are dark. So, expect that for most images the color will be black.
Native and web versions should produce the same HTML
Check starter page templates
Screenshots or screencast
ios-cover-block-dark-mode.mp4