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.

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

Address some warnings in the Gutenberg iOS code used by the React Native layer #49739

Merged
merged 5 commits into from
Apr 13, 2023

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Apr 12, 2023

What? & Why?

I've been touching the code in packages/react-native-bridge/ios as part of my experiment to see whether I can figure out how to distributed gutenberg-mobile as a binary XCFramework for the iOS app(s) to use.

While around there, I noticed a few warnings and addressed to reduce the noise in my IDE.

How?

Refer to the commit messages for details on the individual warnings

Testing Instructions

The code changes here are relevant only to the React Native implementation. I opened a mirror PR on gutenberg-mobile pointing to them to verify CI is green and that the iOS demo app runs locally. See wordpress-mobile/gutenberg-mobile#5659.

Testing Instructions for Keyboard

N.A.

@@ -35,7 +35,7 @@ extension SourceFile {

public func jsScript(with argument: String? = nil) throws -> WKUserScript {
let content = try getContent()
let formatted = String(format: content, argument ?? [])
let formatted = String(format: content, argument ?? "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's appropriate to use "" instead of [] here, given the argument type is String.

In fact, it may also result in a better behavior in case of content expecting an argument and none being provided at callsite:

Screenshot 2023-04-12 at 12 05 30 pm

Comment on lines +51 to +60
static let retrieveHtml = SourceFile(name: "content-functions", type: .js)
static let editorBehavior = SourceFile(name: "editor-behavior-overrides", type: .js)
static let editorStyle = SourceFile(name: "editor-style-overrides", type: .css)
static let wpBarsStyle = SourceFile(name: "wp-bar-override", type: .css)
static let gutenbergObserver = SourceFile(name: "gutenberg-observer", type: .js)
static let injectCss = SourceFile(name: "inject-css", type: .js)
static let retrieveHtml = SourceFile(name: "content-functions", type: .js)
static let insertBlock = SourceFile(name: "insert-block", type: .js)
static let localStorage = SourceFile(name: "local-storage-overrides", type: .json)
static let preventAutosaves = SourceFile(name: "prevent-autosaves", type: .js)
static let gutenbergObserver = SourceFile(name: "gutenberg-observer", type: .js)
static let supportedBlocks = SourceFile(name: "supported-blocks", type: .json)
static let editorBehavior = SourceFile(name: "editor-behavior-overrides", type: .js)
static let wpBarsStyle = SourceFile(name: "wp-bar-override", type: .css)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't addressing a warning, but as per commit message it made it easier for me to scan the contents and compare them with what was in the generated resources folder. I thought it'd made sense to use it here, too.

mokagio added a commit to wordpress-mobile/gutenberg-mobile that referenced this pull request Apr 12, 2023
@mokagio mokagio marked this pull request as ready for review April 12, 2023 05:01
@mokagio
Copy link
Contributor Author

mokagio commented Apr 12, 2023

I can't ask for a review, so pinging @fluiddot @dcalhoun @geriux as folks knowledgeable on the React Native setup. Thanks!

@fluiddot
Copy link
Contributor

I can't ask for a review, so pinging @fluiddot @dcalhoun @geriux as folks knowledgeable on the React Native setup. Thanks!

Oh true, I'll add the reviewers mentioned above for you.

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

These changes make sense to me. It's nice to resolve distracting warnings. Thank you for taking the time to contribute this! 🙇🏻

I verified the Demo editor successfully builds and runs via npm run native ios and via Xcode. I also verified the Demo editor successfully builds and runs using wordpress-mobile/gutenberg-mobile#5659 too.

@mokagio
Copy link
Contributor Author

mokagio commented Apr 13, 2023

Thanks for the in depth review @dcalhoun 🙇‍♂️

I don't have the rights to merge this, who should I ping? cc @fluiddot

@dcalhoun dcalhoun merged commit 595d774 into WordPress:trunk Apr 13, 2023
@github-actions github-actions bot added this to the Gutenberg 15.7 milestone Apr 13, 2023
@mokagio mokagio deleted the mokagio/address-some-ios-warnings branch April 13, 2023 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants