-
Notifications
You must be signed in to change notification settings - Fork 105
first go at readonly #162
base: master
Are you sure you want to change the base?
first go at readonly #162
Conversation
| readonly: { | ||
| type: Boolean, | ||
| value: false, | ||
| observer: '_readonlyChanged' |
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.
I believe we can get rid of this observer, as it can be expensive (this is triggered for all the dropdown menus) and it actually solves a very edge case scenario where someone updates this property while the dropdown is opened.
WDYT?
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.
Thanks for the comment.
Agreed, this is not going to happen very often (readonly property changing while the dropdown is opened), but it should still be the correct behavior, I guess.
What is the actual weight of adding an observer ?
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.
For the actual weight, _readonlyChanged callback is called the first time when readonly is set to its default value, and each time it changes value, which means when is set to true, we have _readonlyChanged called twice.
If you remove the default value for the readonly property, _readonlyChanged will be called only once, when we it is set the first time.
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.
Sounds like a good idea !
98b0a21
|
Hi @christophe-g, the implementation LGTM (left a comment). |
c465bbc to
de05d31
Compare
…while instanciating the component - and improve performance
|
Thanks for the contribution @christophe-g! can you add some unit tests to avoid any future regression? |
Adding a
readonlybehavior forpaper-dropdown-menupaper-menuis set todisabledmodereadonly