[go: nahoru, domu]

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

fix: Call onFinishEditing_ for fields on mobile. #7483

Merged
merged 1 commit into from
Sep 20, 2023

Conversation

laurensvalk
Copy link
Contributor

On the desktop, widgetDispose_ will call onFinishEditing_ on close.

This was missing in the mobile counterpart, so any cleanups in onFinishEditing_ would not be called.

Also update the message string comment while we are touching this code.

The basics

The details

Resolves

On mobile, any cleanups in onFinishEditing_ would not be called.

It is called in widgetDispose_ but not in showPromptEditor_.

Proposed Changes

Call onFinishEditing_ in showPromptEditor_.

Reason for Changes

Make sure onFinishEditing_ works consistently on mobile and desktop.

Test Coverage

Tested on a playground hosted on desktop, viewed on Android mobile device on local network and verified that onFinishEditing_ was not called before but is now.

Documentation

Updated the docstring for the prompt translation.

Additional Information

For consideration (though possibly not needed): Should showPromptEditor_ share more things with widgetDispose_ like forceRerender and firing events or can we assume that setValue does that?

On the desktop, widgetDispose_ will call onFinishEditing_ on close.

This was missing in the mobile counterpart, so any cleanups in onFinishEditing_ would not be called.

Also update the message string comment while we are touching this code.
@laurensvalk laurensvalk requested a review from a team as a code owner September 12, 2023 10:48
@github-actions github-actions bot added the PR: fix Fixes a bug label Sep 12, 2023
Copy link
Contributor
@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Seems sensible to me, but I'm going to get a colleague who is more familiar with this part of the codebase to have a look before approving.

@cpcallen
Copy link
Contributor

So, after discussing this colleagues, we'd like to suggest using the new intermediate edit events vs change events instead of onFinishEditing_, which was always a bit of a hacky solution and might be a candidate for deprecation—see issue #2496 for some discussion about this, and PR #7151 for the implementation of the new events.

Let us know if you think that approach might meet your needs, as we're not sure we want to add further support fro onFinishEditing_ at the current time.

@laurensvalk
Copy link
Contributor Author

Thank you, I'll have a look.

It did seem quite natural to have some control over the final value set by the field (i.e. as argued in #2496 (comment))

If I understand correctly, the suggested approach is to handle events at the block level as opposed to at the field level. Does this mean adding an event handler via setOnChange to every block that uses the field?

This could probably work, but having it close to the field seemed logical thus far. On a related note, I was having some difficulty "merging" several onchange handlers, i.e. adding one to a block that already uses one as part of another extension. It's quite possible there's an obvious method that I missed though :)

@BeksOmega
Copy link
Collaborator

If I understand correctly, the suggested approach is to handle events at the block level as opposed to at the field level. Does this mean adding an event handler via setOnChange to every block that uses the field?

You could do that! Or you could add one handler at the workspace level that iterates over all of the relevant blocks.

I was having some difficulty "merging" several onchange handlers, i.e. adding one to a block that already uses one as part of another extension. It's quite possible there's an obvious method that I missed though :)

Yeah there's not a good way to do this at the moment :/ we have similar problems with other mixins.

Copy link
Collaborator
@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

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

Hello @laurensvalk We discussed this at our team meeting yesterday and decided to accept this change! We still recommend switching over to use the events since assigning to onFinishEditing_ is hacky and not TypeScript compatible. But if this functionality exists we think it makes sense to make it consistent for browsers and mobile =)

Thank you for putting up this change!

[Edit: I'll get this merged after I have a chance to manually test]

@BeksOmega BeksOmega merged commit 3b234c7 into google:develop Sep 20, 2023
11 checks passed
@laurensvalk
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants