-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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] Set native extension to media upload constant file #54928
Conversation
Size Change: 0 B Total Size: 1.62 MB ℹ️ View Unchanged
|
Flaky tests detected in b7d4ea5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6353040768
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fascinating discovery! I could easily see this subtle detail of the translation pipeline resulting in erroneously removed strings in the future. I wonder if there is anyway to guard against this. 🤔
Regardless, thank you for fixing this! 🚀
export const OPTION_TAKE_VIDEO = __( 'Take a Video' ); | ||
export const OPTION_TAKE_PHOTO = __( 'Take a Photo' ); | ||
export const OPTION_TAKE_PHOTO_OR_VIDEO = __( 'Take a Photo or Video' ); | ||
export const OPTION_INSERT_FROM_URL = __( 'Insert from URL' ); | ||
export const OPTION_WORDPRESS_MEDIA_LIBRARY = __( 'WordPress Media Library' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reality, these same strings are utilized elsewhere outside of tests. An alternative to relocating these might be renaming this file to constants.native.js
and replacing the duplicative string literals with references to these constants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcalhoun Yeah, this would be a good alternative. Probably if we rename the file to constants.native.js
the i18n script will identify these strings as native-only ones. I'll apply your suggestion and check if the verify that the i18n script works as expected.
c96b06a
to
b7d4ea5
Compare
Hey @dcalhoun, following this comment thread , I've updated the PR with your suggestion. Let me know if you could take another look at the PR, thanks 🙇 ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified the test plan succeed on an iPhone 15 Pro simulator and Pixel 3 XL emulator. 🚀
What?
Move the test-only media upload constants to the test file where they are being used.Following this suggestion, the media upload constants file now has the native extension and the option constants are being used in the
MediaUpload
component.Why?
This addresses an issue when bundling Gutenberg Mobile related to missing i18n localizations (reference).
How?
The constants related to media options being used inpackages/block-editor/src/components/media-upload/test/index.native.js
have been moved from the constants file (packages/block-editor/src/components/media-upload/constants.js
) to the test.packages/block-editor/src/components/media-upload/constants.js
file has changed tonative.js
.MediaUpload
uses the media options constants to define the media sources displayed in the "Choose media" picker.Testing Instructions
CI jobs
Media options
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A