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.

Sign up for GitHub

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

Jump to bottom

first go at readonly #162

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

first go at readonly #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
894558d
first go at readonly
christophe-g Jun 11, 2016
7775c0e
close menu when setting to readonly
christophe-g Jun 11, 2016
de05d31
allow _selectedItemChanged, otherwise the initial value is not set
christophe-g Jun 11, 2016
98b0a21
removed the default `false` value to allow observer not being called …
christophe-g Jul 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion demo/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ <h4>You can pre-select a value using the <i>selected</i> attribute</h4>
</template>
</demo-snippet>

<h4>You can change the direction in which the menu opens</h4>
<h4>You can change the direction in which the menu opens</h4>
<demo-snippet class="centered-demo">
<template>
<paper-dropdown-menu label="Upwards and to the left!" vertical-align="bottom" horizontal-align="left">
Expand Down
33 changes: 30 additions & 3 deletions paper-dropdown-menu.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
vertical-align="[[verticalAlign]]"
horizontal-align="[[horizontalAlign]]"
vertical-offset="[[_computeMenuVerticalOffset(noLabelFloat)]]"
disabled="[[disabled]]"
disabled="[[_checkDisabled(disabled, readonly)]]"
no-animations="[[noAnimations]]"
on-iron-select="_onIronSelect"
on-iron-deselect="_onIronDeselect"
Expand Down Expand Up @@ -185,7 +185,7 @@
* The error message to display when invalid.
*/
errorMessage: {
type: String
type: String
},

/**
Expand Down Expand Up @@ -242,6 +242,15 @@
verticalAlign: {
type: String,
value: 'top'
},

/**
* If you're using PaperInputBehavior to implement your own paper-input-like
* element, bind this to the `<input is="iron-input">`'s `readonly` property.
*/
readonly: {
type: Boolean,
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

}
},

Expand Down Expand Up @@ -286,7 +295,9 @@
* Show the dropdown content.
*/
open: function() {
this.$.menuButton.open();
if (!this.readonly) {
this.$.menuButton.open();
}
},

/**
Expand Down Expand Up @@ -324,6 +335,16 @@
this.open();
}
},

/**
* Computed the disabled state for menu-button
*
* @param {disabled}
* @param {readonly}
*/
_checkDisabled: function(disabled, readonly) {
return disabled || readonly;
},

/**
* Compute the label for the dropdown given a selected item.
Expand Down Expand Up @@ -375,6 +396,12 @@
if (e) {
e.setAttribute('aria-expanded', openState);
}
},

_readonlyChanged: function(readonly) {
if (readonly) {
this.close();
}
}
});
})();
Expand Down