[RNMobile] Ensure uploaded audio is always visible within Audio block #55627
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
SiobhyB
added
[Type] Bug
added 2 commits
October 25, 2023 16:22
With this commit, the existing "getStylesFromColorScheme" function that applies different styles based on dark/light mode is removed. In its place, a custom function, "determineEditorColorScheme", has been added. In future commits, we'll iterate on this groundwork and "determineEditorColorScheme" will begin to account for any editor styles that come from block-based themes.
|
Size Change: 0 B Total Size: 1.7 MB ℹ️ View Unchanged
|
added 3 commits
October 30, 2023 21:31
To account for a block-based theme's background color, a custom hook has been created called "useEditorColorScheme". The hook works as follows: - Accepts both the light and dark styles for each component. - If there is no block-based theme, with a custom background color, the device's light/dark mode is used to determine the appropriate color scheme. - If there is a block-based theme, the colord.isDark() function is used to test whether the background is dark or not. The result is used to determine the appropriate color scheme to use.
SiobhyB
added
the
Mobile App - Automation
fluiddot
reviewed
Oct 31, 2023
Comment on lines
79
to
81
| if ( typeof editorBackgroundColor === 'undefined' ) { | ||
| return getStylesFromColorScheme( baseStyle, darkStyle ); | ||
| } |
There was a problem hiding this comment.
I noticed that editorBackgroundColor value can also be a String with the value undefined when the color can't be parsed:
You can check this by using the theme Twenty Twenty-Three. In this case, the Audio block is not visible.
There was a problem hiding this comment.
For some reason, I only reproduce the issue I mentioned above when using the iOS simulator. On an actual device, themes and colors are parsed properly 🙃. In any case, adding the condition for the case I mentioned, when values can't be parsed, would still be applicable.
There was a problem hiding this comment.
Noting that I also experienced an illegible Audio block when using the Twenty Twenty-Three theme and dark mode enable only when using a simulator. When compared to a physical device, the editor's background color is different.
| Simulator | Physical Device |
|---|---|
 |
 |
There was a problem hiding this comment.
@fluiddot, @dcalhoun, thank you both! It's strange that this is specific to iOS (not Android) and only reproducible on a simulator. 🤔 I also had trouble reproducing myself, but did not that the editor's background isn't always being displayed on an iOS simulator for me. It feels like there's something more to be dug into here.
Regardless, I've added a check for the "undefined" string, to resolve this case from the code Carlos pointed out: e83f5c6 (also viewable in the refactored function in 190bbd0).
Let me know if that resolves this issue for you!
| @@ -70,14 +71,30 @@ function Player( { | |||
| } | |||
| }; | |||
|
|
|||
| const containerStyle = getStylesFromColorScheme( | |||
| const useEditorColorScheme = ( baseStyle, darkStyle ) => { | |||
There was a problem hiding this comment.
useEditorColorScheme provides functionality that I presume might be needed on other blocks, as choosing styles based on the background seems a common issue across blocks. I wonder if we should move it as a utility related to global styles. WDYT?
There was a problem hiding this comment.
That's a good point! I've gone ahead to refactor the function in 190bbd0, with that reusability in mind.
I also wanted to highlight that this might be a short-term patch, as I plan to re-open the discussion of how we handle block-based/dark-mode colours in the long-term with the team.
dcalhoun
reviewed
Nov 1, 2023
Comment on lines
79
to
81
| if ( typeof editorBackgroundColor === 'undefined' ) { | ||
| return getStylesFromColorScheme( baseStyle, darkStyle ); | ||
| } |
There was a problem hiding this comment.
Noting that I also experienced an illegible Audio block when using the Twenty Twenty-Three theme and dark mode enable only when using a simulator. When compared to a physical device, the editor's background color is different.
| Simulator | Physical Device |
|---|---|
|
|
Co-authored-by: David Calhoun <[email protected]>
As described in the following feedback, there can be times when the editor background colour returns an "undefined" string: #55627 With this commit, that case is accounted for, with the default dark/light styles being applied.
|
Flaky tests detected in e83f5c6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6747684317
|
As per the feedback here, the function may be useful for other instances beyond the audio block, so it has been refactored and moved to a utils file: #55627 (comment) Note, to comply with the rules of hooks, it was also necessary to reorganise the functions logic as part of this refactor: - The "usePreferredColorScheme" replaced "getStylesFromColorScheme" as the function for detecting the device's light/dark mode. - "usePreferredColorScheme" is called after first checking to see if the editor's background is defined.
|
Noting failing unit tests related to the audio block following the last commits, I'm looking into them and will set the PR back as ready for review when I've addressed them. |
"usePreferredColorScheme" was previously used to return whether the device was in dark or light mode but, following a refactor, "usePreferredColorSchemeStyle" is now needed. This is because we're now returning style objects rather than simply the dark/light mode string.
|
Returning to this with fresh eyes, after it fell off my list of priorities, I spotted the bug that was leading to failing tests and fixed it in 51490f0. This PR's ready for review again. :) |
fluiddot
approved these changes
Nov 13, 2023
| * | ||
| * @return {Object} - The combined style object that should be applied to the editor. | ||
| */ | ||
| export const useEditorColorScheme = ( baseStyle, darkStyle ) => { |
There was a problem hiding this comment.
Not a blocker for this PR but eventually it would be great to have unit tests to cover this hook.
There was a problem hiding this comment.
Thanks Carlos, I'll add this to my backlog.
|
@SiobhyB I encountered several React warnings when opening the editor using these changes. However, they were solved after updating with the latest changes from |
This was referenced Nov 23, 2023
|
@SiobhyB FYI I managed to reproduce the issue related to this PR in version |
3 tasks
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.
What?
Fixes #54328, by ensuring the Audio block is always visible when media's been uploaded to it.
Why?
The content of an Audio block with set media is currently invisible when a block-based theme's background colour contrasts with the device's light/dark light mode. For example, if the theme's background colour is white and dark mode is enabled on a device, the Audio block's content will be invisible.
How?
To account for a block based theme's background colour, a custom hook,
useEditorColorScheme, has been created. The hook works as follows:getStylesFromColorScheme()is used to apply the appropriate style based on whether light or dark mode is enabled on a device. This is how the styles were previously applied.colord.isDark()function to determine if its dark or not. Then, the result is used to determine the appropriate colour scheme to use.Testing Instructions
Follow the preparation steps once for each mode (dark/light) and then proceed with the testing steps.
Preparation steps
Testing steps
Additional testing
Testing Instructions for Keyboard
N/A, solely a visual change.
Screenshots or screencast
Dark mode + block based theme with light background⤵️
Light mode + block based theme with dark background⤵️
No changes when dark/light mode matches background⤵️
No changes for non-block based themes⤵️