Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/93: UnlinkCommand should update its state upon editor load #117

Closed
wants to merge 1 commit into from

Conversation

oleq
Copy link
Member

@oleq oleq commented May 12, 2017

Suggested merge commit message (convention)

Fix: UnlinkCommand should update its state upon editor load. Closes ckeditor/ckeditor5#4777.

@oleq oleq requested a review from oskarwrobel May 12, 2017 09:48
@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

Every command should be refreshed initially. This should go to ckeditor5-core.

@oskarwrobel
Copy link
Contributor

oskarwrobel commented May 12, 2017

I'm not sure but I think I was trying to add initial refreshState() to the generic Command class and after this change, some of Undo tests have failed.

@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

They might've been wrong :P

Besides, isn't refreshing in the constructor too early? Is data loaded already then?

@oleq
Copy link
Member Author

oleq commented May 12, 2017

Is data loaded already then?

I checked this and unfortunately, no. But what if we disabled the command by default? The only case I see to suffer because of this is a link at the beginning of a blurred editable and the user wanting to unlink it directly from the toolbar without even entering the editable. Or is there anything more to consider?

OTOH, I just realized this approach is a kind of pitfall that we experienced in v4 and I'd rather avoid it in v5. Maybe it should be fixed at the core level, after all. @oskarwrobel do you have any reference to those issues concerning undo?

@Reinmar
Copy link
Member

Reinmar commented May 12, 2017

I'm not sure I understand your concerns.

The enable/disable mechanism in CKE5 works a bit differently than in CKE4. To avoid the issue that "I want to disable and then enable the command back... but what if someone disabled it in the meantime too!?" issue, in order to disable the command (even from inside) you need to add a listener to the refreshState event which will set evt.data.isEnabled = false and block other listeners.

So, you can absolutely safely call the refreshSate() method at any time. Here, it's just a matter of doing this for every command when the content is ready.

@oleq
Copy link
Member Author

oleq commented May 15, 2017

The issue will be resolved globally in ckeditor5-core.

@oleq oleq closed this May 15, 2017
@oleq oleq deleted the t/93 branch July 11, 2017 14:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unlink toolbar button is enabled when selection is not inside link element
3 participants