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

Remove behavior & added only the essential code, fix imports, fix sty… #16

Closed
wants to merge 1 commit into from

Conversation

herberthobregon
Copy link
Contributor

  1. Remove behavior & added only the essential code to work
  2. Fix imports
  3. Fix styles to have a behavior like a paper-input & that the paper-dropdown occupies its own space and does not stretch it inside
  • [*] Stable

Copy link
Owner

@pushkar8723 pushkar8723 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also updated the 3.0 branch which addresses some of the issues I pointed out in comments. Please rebase your commit and make changes.

@@ -85,7 +86,7 @@ import PaperDropdownBehavior from './paper-dropdown-element-behavior';
* @element paper-dropdown
* @demo demo/index.html
*/
class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Polymer.Element) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixin is a way in polymer to abstract out code / functionality which can be overridden by someone using the component.
Example: the validation in IronValidatableBehavior which we override for our custom validation. Which in turn can be overridden by someone using paper-dropdown. Also, removing the IronValidatableBehavior should result in form validation to fail. Did you test it?

const re = new RegExp(searchText, "gi");
return (re.exec(currentValue) != null);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You didn't even add _getValidatity which means this PR is not tested for form validation which is very essential for any input component.

display: inline-block;
outline: none;
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this required?

@@ -303,6 +332,9 @@ class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Poly
:host * {
outline: none;
}
paper-dropdown-menu{
display: block;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paper-dropdown is supposed to be just a wrapper around paper-dropdown-menu and hence should not add any custom css to it. If need arises, user can use --paper-dropdown-menu css mixin to apply their custom css.

@@ -422,7 +454,7 @@ class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Poly
if (this.multi) {
this.set('value', selectedItems.map(item => this._getItemValue(item)));

const { items } = this.$.list;
const {items} = this.$.list;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are going to stick to airbnb code style from now on. This change will cause lint issues.
You must have got error which trying to commit, please don't ignore lint error message as soon I will add travis build process to have all these checks.

@@ -516,7 +548,7 @@ class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Poly
* @private
*/
_updateSelected(value) {
const { items } = this.$.list;
const {items} = this.$.list;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@@ -541,7 +573,7 @@ class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Poly
* @private
*/
_updateValue(selected) {
const { items } = this.$.list;
const {items} = this.$.list;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@@ -561,10 +593,10 @@ class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Poly
*/
_onOpenedChanged(opened) {
if (opened) {
this.dispatchEvent(new CustomEvent('open', { detail: null, bubbles: false }));
this.dispatchEvent(new CustomEvent('open', {detail: null, bubbles: false}));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Save as above.

} else {
this.set('_searchText', '');
this.dispatchEvent(new CustomEvent('close', { detail: null, bubbles: false }));
this.dispatchEvent(new CustomEvent('close', {detail: null, bubbles: false}));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@@ -587,7 +619,7 @@ class PaperDropDown extends Polymer.mixinBehaviors([PaperDropdownBehavior], Poly
* @private
*/
_filter(searchText) {
const { items } = this.$.list;
const {items} = this.$.list;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

@pushkar8723
Copy link
Owner

Closing this because of inactivity.

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

Successfully merging this pull request may close these issues.

None yet

2 participants