Overwrite Missing block UI Text #2610
Conversation
|
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
|
Hey @etoledom 👋 |
# Conflicts: # gutenberg
| const unsupportedBlocksButton = shouldOverwriteButtonTitle | ||
| ? __( 'Open Jetpack Security settings' ) | ||
| : undefined; |
There was a problem hiding this comment.
Nitpick: It seems like instead of using the extra shouldOverwriteButtonTitle variable, we could use the if-else statement above to set unsupportedBlocksButton.
There was a problem hiding this comment.
This would make us declare unsupportedBlocksButton as let right?
I thought shouldOverwriteButtonTitle name would also give some context of the reasoning for the undefined.
Would you say something like this?
let unsupportedBlocksButtonTitle = undefined;
if ( unsupportedBlockEditor.supported === false && unsupportedBlockEditor.switchable ) {
unsupportedBlocksButtonTitle = __( 'Open Jetpack Security settings' );
}There was a problem hiding this comment.
Yes I think I had something like that in mind.
src/ui-strings.js
Outdated
| const unsupportedBlockEditor = { | ||
| supported: | ||
| getSettings( 'capabilities' ).unsupportedBlockEditor === true, | ||
| switchable: | ||
| getSettings( 'capabilities' ).unsupportedBlockEditorSwitch === true, | ||
| }; |
There was a problem hiding this comment.
Nitpick: An alternative here could be to use:
const {
unsupportedBlockEditor = false,
unsupportedBlockEditorSwitch = false
} = getSettings( 'capabilities' ).unsupportedBlockEditor;
There was a problem hiding this comment.
unsupportedBlockEditor.supported and unsupportedBlockEditor.switchable I think gives a nice context on their usage. Like:
if (
unsupportedBlockEditor.supported === false &&
unsupportedBlockEditor.switchable
)vs
if (
unsupportedBlockEditor === false &&
unsupportedBlockEditorSwitch
)What do you think?
There was a problem hiding this comment.
(By the way, I think this whole file is a mess anyway, and not much extendible if we add more strings in the future. We will probably split it in the future)
There was a problem hiding this comment.
Ideally, those attributes (unsupportedBlockEditor and unsupportedBlockEditorSwitch) would be descriptive enough on their own so that they can be deconstructed and used as-is without needing to be renamed. But I know it's hard here to find a good name, so I'm happy to leave it as-is if you want to.
| const { getSettings } = select( 'core/block-editor' ); | ||
| const unsupportedBlockEditor = { | ||
| supported: | ||
| getSettings( 'capabilities' ).unsupportedBlockEditor === true, |
There was a problem hiding this comment.
Just asking out of curiosity. We have a lot of explicit bool checks in this file (i.e., someValue === true and someValue === false). Are we doing that to guard against a cases where someValue could be some other truth-y or false-y values, or is it a style preference to do the explicit bool check instead of doing something like someValue (or even !!someValue if we needed a bool) and !someValue, or maybe there's another reason?
Just asking because I tend to use the !s when I can (I find them more readable), and I'm curious if there are considerations I may not be considering when I do that. I'm not asking because I think we need to change it here. 🙂
There was a problem hiding this comment.
Couldn't we have achieved that feature by just adding a hook in missing/edit.native.js:
const missingBlockDetail = applyFilters( 'native.missing_block_detail', __( 'We are working hard to add more blocks with each release.' ) );
...
<Text style={ [ infoTextStyle, infoDescriptionStyle ] }>
{ missingBlockDetail }
</Text>Then in gb-mobile:
addFilter( 'native.missing_block_detail', ( defaultValue ) => {
// uiStrings() logic goes here
} );
?
Same for the action button.
|
|
||
| export const uiStrings = () => { | ||
| const { getSettings } = select( 'core/block-editor' ); | ||
| const unsupportedBlockEditor = { |
There was a problem hiding this comment.
Nitpick: I think just using 2 variables instead of grouping them in an object is just simpler and easier to understand, especially since you're assigning an unsupportedBlockEditor to a supported property
| }; | ||
| }; | ||
|
|
||
| addStrings( uiStrings ); |
There was a problem hiding this comment.
Nitpick: I would expect that, given its name, addStrings would accept a list of strings as parameter. The fact that it accepts a function returning a mapping of (string id, string value) makes it a bit hard to grasp imo.
@Tug - Nice tip! Indeed looks a lot simpler. The only thing we need from it is that it has to refresh and return the correct String after updating the capabilities. |
WPiOS PR: wordpress-mobile/WordPress-iOS#14880
WPAndroid PR: wordpress-mobile/WordPress-Android#13011
gutenberg PR: WordPress/gutenberg#25238
This PR implements the changes from WordPress/gutenberg#25238 and overwrites the default Missing block alert text with text that references WordPress.com and Jetpack.
This also allows us to change these texts dynamically depending on the UBE-related capabilities sent from the native apps, after calling
updateCapabilities().To test:
Please check wordpress-mobile/WordPress-iOS#14880
PR submission checklist: