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

core: fix logic in getToggledHandler #5906

Closed
wants to merge 1 commit into from

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Aug 9, 2019

What it does

Fix logic issue when looking for a toggled command handler.

Follow up of #5894 (review)

How to test

Register a command handler not defining a isToggle function.
Before this patch it would not be shown as toggled.
With this patch it should be shown as toggled.

Review checklist

Reminder for reviewers

If `isToggled` isn't defined, the command would be considered as not
toggled.

We want it to be seen as toggled by default instead.

Signed-off-by: Paul Maréchal <[email protected]>
@vince-fugnitto
Copy link
Member

vince-fugnitto commented Aug 9, 2019

I think we can update the command.spec.ts to handle the isVisible, isToggled and isEnabled cases so at least we may catch some regressions.

It should be possible to test the internal state of the methods.

@vince-fugnitto
Copy link
Member

vince-fugnitto commented Aug 9, 2019

The logic seems incorrect, please see below from testing:

Screen Shot 2019-08-09 at 12 43 21 PM Screen Shot 2019-08-09 at 12 46 31 PM

@paul-marechal paul-marechal deleted the mp/handler-is-toggled branch August 9, 2019 17:48
@paul-marechal
Copy link
Member Author

Turns out the logic is fine the way it is now :)

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.

2 participants