-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
[Sidebar] Respect a change of closable setting #335
[Sidebar] Respect a change of closable setting #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The other way around doesn't work either. If you set closable to false, open it, then set closable to true from inside the sidebar doesn't bind the Fomantic-UI/src/definitions/modules/sidebar.js Lines 176 to 184 in b318944
https://jsfiddle.net/Korinu/5obdax1z/ |
@ColinFrick Then i suggest to always bind the |
…ide method centrally
@ColinFrick As suggested, the clickaway is now bound in any case. This was the easiest (in my pov cleanest) solution. otherwise i had to bind/unbind the clickway every time the closable settings changes and i feared other bugs then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s fine
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
The
closable
setting is ignored in sidebar when the setting was changed after initialization.Testcase
-> Sidebar should not close
Unfixed (original SUI issue)
https://jsfiddle.net/r7fws0nn
Fixed
https://jsfiddle.net/r7fws0nn/23/
Closes
Semantic-Org/Semantic-UI#5329