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 problem with email inline link editor disappearing #2413

Merged
merged 2 commits into from
Dec 8, 2024

Conversation

awarn
Copy link
Collaborator

@awarn awarn commented Dec 7, 2024

Description

Our own event listening for changes to selection (and then updating tool state) triggered too early. Editor.js runs through closing and opening the toolbar every time the selection changes, with a call to checkState function meant to decide if we should open it (again).

Previously we would run our own update, where the logic for deciding to have toolbar open was, which displayed it. Then Editor.js would close it and call checkState, but because there was no code there for keeping it open, it would remain close. So the link tool would just briefly flash open.

Changes

  • Email inline link editor now visible when selecting text after link creation.
  • Change inline variable editor to also use correct lifecycle.

Related issues

Resolves #2176

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

Very impressive how you were able to figure this one out. But now typescript is complaining. Can you have a look?

@awarn
Copy link
Collaborator Author

awarn commented Dec 7, 2024

Not a great fix, even if it worked. Docs for checkState() do not have any reference to a return type (other than void), but the type requires boolean. I have tried returning both true and false, and there is no difference in behavior what I can tell.

@richardolsson
Copy link
Member

Not a great fix, even if it worked. Docs for checkState() do not have any reference to a return type (other than void), but the type requires boolean. I have tried returning both true and false, and there is no difference in behavior what I can tell.

Good enough! We are hoping to replace Editor.js soonish (because we've been having a lot of problems with it) so less-than-perfect solutions are more than good enough at this point. 😊

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

Nice work figuring this out! I'm not sure why it was so complex before, if it could be so much simpler.

I know we struggled with it a lot, to get it to work in both Chrome and Firefox, which this PR also does. I can see that this solution is not working in Safari, but neither was main, so that's not the reason for the added complexity.

Seeing as how this seems to be working better then main, I'm going to go ahead and merge. I can't wait to replace this Editor.js and all of it's weirdness with something else soon. 😊

@richardolsson richardolsson merged commit aaa7a30 into zetkin:main Dec 8, 2024
6 checks passed
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.

Links (inline tool) in email composer behaves erratically
2 participants