-
Notifications
You must be signed in to change notification settings - Fork 107
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
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
readonly
behavior forpaper-dropdown-menu
paper-menu
is set todisabled
modereadonly