[RNMobile] Add Contrast Checker to Text-Based Blocks #34902
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
With this commit, .native files have been created for the ContrastChecker component. The component's main index.native.js was copied over from the main web version, with web-specific pieces (e.g.<div> elements) removed so that it can be called from other mobile files without error.
|
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
added 6 commits
September 20, 2021 13:02
This commit takes the web's ColorPanel hook and adapts it for native use. Specifically, while the web version relies on ComputedStyles to fetch a block's background/text colour, we need to use other means for native. Here, we're using the useGlobalStyles, which will fetch a block's background and text colour when they're explicitly set via the global style settings.
With this commit, the ContrastChecker component (which is being passed via "children") is added to the Color panel.
With this commit, useEffect and useState hooks are introduced to detect changes to a block's text and/or background colors.
|
👋 @iamthomasbishop, @kyleaparker, would I be able to get your feedback on the current design of the notice? I used
Looking forward to hearing any feedback! |
SiobhyB
added
Mobile App - i.e. Android or iOS
2 tasks
SiobhyB
removed
[Status] In Progress
|
👋 @geriux, would you have time to review this? As noted in the description, this PR only adds the contrast checker to the main colour settings panel, as I thought it'd be best to keep the scope fairly narrow for now. It could be added to the individual colour picker screens in future iterations. Let me know if anything is unclear in the code or description! Looking forward to hearing any feedback. |
Hey 👋 Of course! looking forward to review/test this feature 🎉 |
|
@SiobhyB This is looking pretty solid, nice work! At first glance there are a few of things I'm seeing, for which I'll make proposals for:
Here's what that would look like all together: As a separate note: in the long-term, it might be nice to try also adding a contrast warning on the sub-sheet where you select an individual color, but this is a great start. Thanks for working on this and great job! It'll be a really handy feature that our users will love! 😍 |
geriux
reviewed
Dec 13, 2021
There was a problem hiding this comment.
Hey @SiobhyB 👋 great work adding this feature! I tested different scenarios and its working correctly 🎉 . I left a couple of minor comments, let me know what you think.
By the way, it'd be great if you could update from the latest changes in trunk so I can also test the higlighting text feature. Thanks!
packages/block-editor/src/components/contrast-checker/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/contrast-checker/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/contrast-checker/style.native.scss
Outdated
Show resolved
Hide resolved
added 3 commits
December 20, 2021 14:59
'tinycolor' usage has recently been updated in core with 'colord', so this commit reflects that update.
As we're using a functional component, the use of hooks are preferable to HOCs.
As we're using a functional component, the use of hooks are preferable to HOCs. The 'withPreferredColorScheme' HOC is therefore replaced with the 'usePreferredColorSchemeStyle' as part of this commit.
added 3 commits
December 21, 2021 18:19
This commit includes changes to the styling of the contrast checker message. As part of the changes, a warning icon is added and the colour of the icon/text is updated to yellow. As part of these changes, the class names of each element has been updated to match with the BEM methodology, as per core standards.
'separatorType' is currently not doing anything and was added by mistake as part of the last commit, it's removed with this commit.
By placing the contrast checker in its own PanelBody, we remove its top border, following design feedback.
|
@geriux, @iamthomasbishop, thank you for your reviews so far! 🙇♀️ I've gone ahead to implement the suggested changes to the code and have also updated the design as follows:
Note, I took the values for Let me know how this looks to you and if you have any other suggested changes. |
|
Hey @SiobhyB 👋 Thanks for the changes! The code looks good! I tested it on both platforms and it works as expected 🎉 . I did notice on Android that the icon of the warning message is too close to the left margin:
|
|
Ah, I slipped up on double-checking Android, thanks for spotting that @geriux! I've gone ahead to update the CSS and here's how things look across both platforms now: Android
iOS
I'm about to head off AFK for a week but looking forward to continuing to work on this in the New Year, thanks for all your help so far 🙇♀️ |
geriux
approved these changes
Dec 23, 2021
|
@geriux, thanks so much! 🙇♀️ @iamthomasbishop, I'll wait to hear back on whether the updated designs match up to what you were wanting before shipping :D |
|
@SiobhyB changes look great, thank you! Feel free to ship it when ready! 👏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #33396
Gutenberg Mobile:wordpress-mobile/gutenberg-mobile#4357Description
With this PR, a contrast checker is added to the text-based blocks in the native apps. This checker will display the following notice when users pick a custom text/background colours with poor contrast:
How has this been tested?
Testing Notes
Screenshots
Video
contrast.mov
Types of change
New feature (non-breaking change which adds functionality), with the following high-level overview of the code:
components/contrast-checkerhas been created for use with the apps.hooks/color-panelhas been created. This was necessary in order to detect active text/background colours from the apps.components/contrast-checkeris called fromhooks/color-panel.nativein order to run the checker within blocks that call that hook.Checklist:
*.native.jsfiles for terms that need renaming or removal).