Skip to content

Conversation

@jkmassel
Copy link
Contributor

@jkmassel jkmassel commented Jul 16, 2024

Adds a basic Android app for testing

dcalhoun and others added 4 commits August 20, 2024 10:17
* build: Add prettier

* build: Add @wordpress/prettier-config

Align code formatting with the Gutenberg project.

* style: Format code with `@wordpress/prettier-config`

Align with the Gutenberg project to mitigate style conflicts.
Copy link

@geriux geriux left a comment

Choose a reason for hiding this comment

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

Thank you for working on adding the Android Demo app!

I noticed that the WebView's height doesn't adapt when the keyboard is opened causing the Toolbar to not be visible, as well as when you add content or add new paragraphs, the content gets hidden under the keyboard.

I tested two different simulators to verify it wasn't something specific to a device's config. Can you replicate this issue?

"type": "module",
"scripts": {
"dev": "vite",
"dev": "vite --host",
Copy link

Choose a reason for hiding this comment

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

Nice! I like having the host arg as the default.

const value = { message: message, body: parameters };
window.webkit.messageHandlers.editorDelegate.postMessage(value);
export function editorLoaded() {
console.log("Firing JS editorLoaded event");
Copy link

Choose a reason for hiding this comment

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

If this was just for testing, could we remove it?

console.log("Firing JS editorLoaded event");

if(window.editorDelegate) {
editorDelegate.onEditorLoaded();
Copy link

Choose a reason for hiding this comment

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

Although by itself editorDelegate exists, to prevent ESLint formatting warnings, could we update this to:

Suggested change
editorDelegate.onEditorLoaded();
window.editorDelegate.onEditorLoaded();


export function onBlocksChanged(isEmpty = false) {
if(window.editorDelegate) {
editorDelegate.onBlocksChanged(isEmpty);
Copy link

Choose a reason for hiding this comment

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

Although by itself editorDelegate exists, to prevent ESLint formatting warnings, could we update this to:

Suggested change
editorDelegate.onBlocksChanged(isEmpty);
window.editorDelegate.onBlocksChanged(isEmpty);


export function showBlockPicker() {
if(window.editorDelegate) {
editorDelegate.showBlockPicker();
Copy link

Choose a reason for hiding this comment

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

Same as above:

Suggested change
editorDelegate.showBlockPicker();
window.editorDelegate.showBlockPicker();

Copy link

Choose a reason for hiding this comment

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

I noticed some formatting warnings when opening this file. I usually set Visual Studio Code to apply formatting fixes when I save changes, but we could also do npm run format.

Can we please fix the formatting warnings before merging these changes? Thanks!

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.

Thanks for completing this. Very helpful. 👍🏻 The Android demo app worked for me.

I left a few suggestions, but feel free to disregard them as items we could complete later.

As @geriux mentioned, I recommend we format the JavaScript (npm run format) before merging. Eventually, we'll set up a linter to avoid needing to discuss that particular subject during code review.

Comment on lines +61 to +71
// Production mode – load the assets from the app bundle – you'll need to copy
// this value out of the `dist` directory after building GutenbergKit
this.loadUrl("file:///android_asset/index.html")

// Dev mode – you can connect the app to a local dev server and have it refresh as
// changes are made. To start the server, run `make dev-server` in the project root
// directory.
//
// You'll need to set this to the IP address of your local machine
//
// this.loadUrl("http://192.168.5.248:5173/")
Copy link
Member

Choose a reason for hiding this comment

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

Long term, I suppose we might mirror iOS' setup where a GUTENBERG_EDITOR_URL environment variable toggles dev mode and sets the IP and port. I don't feel like we have to address this before merging.

Comment on lines +61 to +62
// Production mode – load the assets from the app bundle – you'll need to copy
// this value out of the `dist` directory after building GutenbergKit
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment still true now that a Makefile exists that copies the built assets?

var body: some View {
NavigationView {
EditorView()
EditorView(editorURL: URL(string: "http://localhost:5173/")!)
Copy link
Member

Choose a reason for hiding this comment

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

Should this rely upon the GUTENBERG_EDITOR_URL environment variable instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably – this could use a bigger refactor anyway so I'm comfortable leaving it for a future change

@jkmassel
Copy link
Contributor Author

I think I've addressed most of the feedback – there are some other forthcoming PRs that'll add an Android module and some other stuff so some of the more structural stuff should probably be addressed there.

@jkmassel jkmassel requested review from dcalhoun and geriux August 21, 2024 18:43
@jkmassel jkmassel enabled auto-merge (squash) August 21, 2024 18:51
Co-authored-by: David Calhoun <github@davidcalhoun.me>
@jkmassel jkmassel requested a review from dcalhoun August 21, 2024 19:29
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.

Latest changes work well for me on Android and iOS. 🚀 Thank you!

@jkmassel jkmassel merged commit 0c3e5f1 into trunk Aug 21, 2024
@jkmassel jkmassel deleted the add/android branch August 21, 2024 19:44
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.

4 participants