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

[RNMobile] Ensure uploaded audio is always visible within Audio block #55627

Merged
merged 12 commits into from
Nov 16, 2023
Merged

[RNMobile] Ensure uploaded audio is always visible within Audio block #55627

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4a307dd
Update CHANGELOG
Oct 25, 2023
1548559
Update ticket reference in CHANGELOG
Oct 25, 2023
1fcb337
Refactor logic for dark/light mode styling
Oct 26, 2023
f324625
Account for block-based theme's background color
Oct 30, 2023
3906599
Merge branch 'trunk' into rnmobile/audio-block-visibility
Oct 30, 2023
531089c
Fix positioning of CHANGELOG entry
Oct 30, 2023
4c29c2f
Iterate on CHANGELOG description
Nov 3, 2023
bab1437
Merge branch 'trunk' into rnmobile/audio-block-visibility
Nov 3, 2023
e83f5c6
Fallback to device style if theme styles undefined
Nov 3, 2023
190bbd0
Refactor 'useEditorColorScheme' for usability
Nov 3, 2023
51490f0
Call correct hook for returning color scheme style
Nov 10, 2023
771a02f
Merge branch 'trunk' into rnmobile/audio-block-visibility
Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 26 additions & 12 deletions packages/components/src/mobile/audio-player/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ import {
Platform,
} from 'react-native';
import { default as VideoPlayer } from 'react-native-video';
import { colord } from 'colord';

/**
* WordPress dependencies
*/
import { View } from '@wordpress/primitives';
import { Icon } from '@wordpress/components';
import { Icon, useGlobalStyles } from '@wordpress/components';
import { withPreferredColorScheme } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';
import { audio, warning } from '@wordpress/icons';
Expand Down Expand Up @@ -70,14 +71,30 @@ function Player( {
}
};

const containerStyle = getStylesFromColorScheme(
const useEditorColorScheme = ( baseStyle, darkStyle ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const globalStyles = useGlobalStyles();
const editorColors = globalStyles?.baseColors?.color;
const editorBackgroundColor = editorColors?.background;

if ( typeof editorBackgroundColor === 'undefined' ) {
return getStylesFromColorScheme( baseStyle, darkStyle );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that editorBackgroundColor value can also be a String with the value undefined when the color can't be parsed:

gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js

Line 34 in 531089c

const UNKNOWN_VALUE = 'undefined';
)

gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js

Lines 226 to 236 in 531089c

if ( variable === 'preset' ) {
stylesBase = stylesBase.replace( regex, ( _$1, $2 ) => {
const path = $2.split( '--' );
const mappedPresetValue = mappedValues[ path[ 0 ] ];
if ( mappedPresetValue && mappedPresetValue.slug ) {
const matchedValue = Object.values(
mappedPresetValue.values ?? {}
).find( ( { slug } ) => slug === path[ 1 ] );
return matchedValue?.[ mappedPresetValue.slug ];
}
return UNKNOWN_VALUE;

gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js

Lines 260 to 268 in 531089c

if ( variable === 'var' ) {
stylesBase = stylesBase.replace( varRegex, ( _$1, $2 ) => {
if ( mappedValues?.color ) {
const matchedValue = mappedValues.color?.values?.find(
( { slug } ) => slug === $2
);
return `"${ matchedValue?.color }"`;
}
return UNKNOWN_VALUE;

You can check this by using the theme Twenty Twenty-Three. In this case, the Audio block is not visible.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Illegible Audio block details with Twenty Twenty-Three theme and dark mode on simulator Legible Audio block details with Twenty Twenty-Three theme and dark mode on physical iPhone SE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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!


const isEditorBackgroundDark = colord( editorBackgroundColor ).isDark();

return isEditorBackgroundDark
? { ...baseStyle, ...darkStyle }
: baseStyle;
};

const containerStyle = useEditorColorScheme(
styles.container,
styles.containerDark
);

const iconStyle = getStylesFromColorScheme( styles.icon, styles.iconDark );
const iconStyle = useEditorColorScheme( styles.icon, styles.iconDark );

const iconDisabledStyle = getStylesFromColorScheme(
const iconDisabledStyle = useEditorColorScheme(
styles.iconDisabled,
styles.iconDisabledDark
);
Expand All @@ -89,7 +106,7 @@ function Player( {
...( isDisabled && iconDisabledStyle ),
};

const iconContainerStyle = getStylesFromColorScheme(
const iconContainerStyle = useEditorColorScheme(
styles.iconContainer,
styles.iconContainerDark
);
Expand All @@ -99,17 +116,14 @@ function Player( {
...( isIOS ? styles.titleContainerIOS : styles.titleContainerAndroid ),
};

const titleStyle = getStylesFromColorScheme(
styles.title,
styles.titleDark
);
const titleStyle = useEditorColorScheme( styles.title, styles.titleDark );

const uploadFailedStyle = getStylesFromColorScheme(
const uploadFailedStyle = useEditorColorScheme(
styles.uploadFailed,
styles.uploadFailedDark
);

const subtitleStyle = getStylesFromColorScheme(
const subtitleStyle = useEditorColorScheme(
styles.subtitle,
styles.subtitleDark
);
Expand All @@ -119,7 +133,7 @@ function Player( {
...( isUploadFailed && uploadFailedStyle ),
};

const buttonBackgroundStyle = getStylesFromColorScheme(
const buttonBackgroundStyle = useEditorColorScheme(
styles.buttonBackground,
styles.buttonBackgroundDark
);
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ For each user feature we should also add a importance categorization label to i
-->

## Unreleased
- [*] Audio block: Fix visibility of upload audio [#55627]
SiobhyB marked this conversation as resolved.
Show resolved Hide resolved

## 1.107.0
- [*] Social Icons: Fix visibility of inactive icons when used with block based themes in dark mode [#55398]
Expand Down
Loading