Skip to content

Conversation

@john-traas
Copy link
Contributor

@john-traas john-traas commented Nov 18, 2024

Fixes: https://github.com/Lundalogik/crm-feature/issues/4476

This uses ref forwarding to override the default focus method allowing a consumer to set focus on the prosemirror editor view.

@github-actions
Copy link

Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3304/

Copy link
Contributor

@civing civing left a comment

Choose a reason for hiding this comment

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

I think it's better to let the consumer decide when to set focus and how. I'm not sure we always want to set the focus back to the text editor when we have a stopTrigger event. It should be up to the consumer

@john-traas
Copy link
Contributor Author

I think it's better to let the consumer decide when to set focus and how. I'm not sure we always want to set the focus back to the text editor when we have a stopTrigger event. It should be up to the consumer

Should we instead have a specific setFocus event that the text editor can listen for?

@civing
Copy link
Contributor

civing commented Nov 19, 2024

I think it's better to let the consumer decide when to set focus and how. I'm not sure we always want to set the focus back to the text editor when we have a stopTrigger event. It should be up to the consumer

Should we instead have a specific setFocus event that the text editor can listen for?

Does it work to override the focus() method in limel-prosemirror-adatpor and call this.view.focus() ?

@john-traas
Copy link
Contributor Author

I think it's better to let the consumer decide when to set focus and how. I'm not sure we always want to set the focus back to the text editor when we have a stopTrigger event. It should be up to the consumer

Should we instead have a specific setFocus event that the text editor can listen for?

Does it work to override the focus() method in limel-prosemirror-adatpor and call this.view.focus() ?

The setFocus function on the limel-prosemirror-adapter does that:

public setFocus() {
    this.view?.focus();
}

I could try ref forwarding from the limebb-text-editor -> limel-text-editor -> prosemirror-adapter to call the setFocus function from the consumer.

@civing
Copy link
Contributor

civing commented Nov 19, 2024

I think it's better to let the consumer decide when to set focus and how. I'm not sure we always want to set the focus back to the text editor when we have a stopTrigger event. It should be up to the consumer

Should we instead have a specific setFocus event that the text editor can listen for?

Does it work to override the focus() method in limel-prosemirror-adatpor and call this.view.focus() ?

The setFocus function on the limel-prosemirror-adapter does that:

public setFocus() {
    this.view?.focus();
}

I could try ref forwarding from the limebb-text-editor -> limel-text-editor -> prosemirror-adapter to call the setFocus function from the consumer.

I tried to override the public focus method and it works great. I'm not sure if this is the most canonical way of doing it though. But it is simple and works 😄

Screen.Recording.2024-11-19.at.18.02.17.mov

limel-text-editor

    connectedCallback() {
        this.host.focus = this.focus.bind(this);
    }

        return [
            <limel-prosemirror-adapter
                ref={(el) =>
                    (this.prosemirrorAdapter =
                        el as HTMLLimelProsemirrorAdapterElement)
                }
                aria-placeholder={this.placeholder}
                contentType={this.contentType}
                onChange={this.handleChange}
                customElements={this.customElements}
                value={this.value}
                aria-controls={this.helperTextId}
                id={this.editorId}
                tabindex={this.disabled ? -1 : undefined}
                aria-disabled={this.disabled}
                language={this.language}
                triggerCharacters={this.triggers}
            />,
            this.renderPlaceholder(),
            this.renderHelperLine(),
        ];

limel-prosemirror-adatpor

    public connectedCallback() {
        if (this.view) {
            this.initializeTextEditor();
        }

        this.host.addEventListener(
            'open-editor-link-menu',
            this.handleOpenLinkMenu,
        );
        this.host.focus = this.focus.bind(this);
    }

    public focus() {
        console.log('Focus is great!');
        this.view?.focus();
    }

@john-traas john-traas force-pushed the set-focus-on-trigger-stop branch from c82e58d to b9784ed Compare November 20, 2024 13:19
@john-traas
Copy link
Contributor Author

john-traas commented Nov 20, 2024

@civing I've dropped the stopTrigger function completely for now - it's easy to implement if we need it.

I've added ref forwarding so we can access the focus function on the prosemirror-adapter.

I'm not sure about this approach though.

  • overriding the native focus method might not be a good idea 😅
  • if we use ref forwarding but don't override the native focus we'll need a small separate interface in each component, so let's not do that.

My personal preference would be to have our own event, have the consumer trigger it by adding an interface, it could be in it's own very small plugin, and when that function is called it sends the event that the prosemirror-adapter is listening for and calls the setFocus() function which would essentially be the same as the first iteration but without coupling it to the triggerStop event.

@john-traas john-traas force-pushed the set-focus-on-trigger-stop branch from b9784ed to 36d30f0 Compare November 20, 2024 14:38
@john-traas john-traas changed the title feat(text editor): reset focus on triggerStop event feat(text editor): use ref forwarding to overwrite focus method Nov 20, 2024
@civing civing force-pushed the set-focus-on-trigger-stop branch from 36d30f0 to 4e28a35 Compare November 20, 2024 16:40
@john-traas john-traas changed the title feat(text editor): use ref forwarding to overwrite focus method feat(text editor): use ref forwarding and 'onFocus' to delegate focus to prosemirror-adapter Nov 20, 2024
@civing civing changed the title feat(text editor): use ref forwarding and 'onFocus' to delegate focus to prosemirror-adapter Delegate the focus inside the text editor Nov 21, 2024
Makes sure to delegate the focus to prosemirror if the text editor get's
the focus.
@civing civing force-pushed the set-focus-on-trigger-stop branch from b5fa744 to 9797f0c Compare November 21, 2024 13:38
@civing civing enabled auto-merge (rebase) November 21, 2024 13:38
@civing civing merged commit fc813b2 into main Nov 21, 2024
10 checks passed
@civing civing deleted the set-focus-on-trigger-stop branch November 21, 2024 13:42
@lime-opensource
Copy link
Collaborator

🎉 This PR is included in version 37.70.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants