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

first go at readonly #162

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

christophe-g
Copy link

Adding a readonly behavior for paper-dropdown-menu

  • paper-menu is set to disabled mode
  • the menu closes when setting to readonly

readonly: {
type: Boolean,
value: false,
observer: '_readonlyChanged'
Copy link
Member

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?

Copy link
Author

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 ?

Copy link
Member

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.

Copy link
Author

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

@valdrinkoshi
Copy link
Member

valdrinkoshi commented Jun 30, 2016

Hi @christophe-g, the implementation LGTM (left a comment).
Tests are failing because you need to sync your branch with master.
Could you also add some unit tests so we avoid future regressions?

…while instanciating the component - and improve performance
@valdrinkoshi
Copy link
Member

Thanks for the contribution @christophe-g! can you add some unit tests to avoid any future regression?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants