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

28 - Update to Polymer 2.x - part 3 #31

Merged
merged 5 commits into from
Nov 16, 2018
Merged

28 - Update to Polymer 2.x - part 3 #31

merged 5 commits into from
Nov 16, 2018

Conversation

dotpointer
Copy link
Contributor

@dotpointer dotpointer commented Nov 15, 2018 •
edited
Loading

Update to Polymer 2.x - part 3.

  • Removing unnecessary constructor.
  • Replacing fire with dispatchEvent in tests.
  • Removing is attribute from style in custom-style.

Not sure if the dom-repeat template should be wrapped in dom-repeat.

I am also welcoming suggestions on more things to convert in this component, if there are any.

Issue is #28.

@dotpointer
Update to Polymer 2.x - part 2.
e19b486 
* Update component versions.

Refs #28
@dotpointer
Update to Polymer 2.x - part 3.
ade7c0c 
* Removing unnecessary constructor.
* Replacing fire with dispatchEvent in tests.
* Removing is attribute from style in custom style.

Refs #28
@dotpointer dotpointer self-assigned this Nov 15, 2018
@Wurper
Copy link
Contributor

Wurper commented Nov 15, 2018

this.async can be replaced with Polymer.Async.microTask.run(() => this.someMethod());
And import <link rel="import" href="/bower_components/polymer/lib/utils/async.html">

Wurper
Wurper reviewed Nov 15, 2018
View reviewed changes
test/basic.html Outdated
@@ -30,7 +30,7 @@
// Set input value
autocomplete.text = value.toString();
// Trigger create suggestions
suggestions._input.fire('keyup');
suggestions.dispatchEvent(new CustomEvent('keyup', { bubbles: true, composed: true }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for removing _input ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this was a mistake that also halted the tests. Added it back.

Wurper
Wurper reviewed Nov 15, 2018
View reviewed changes
test/basic.html Outdated
@@ -118,7 +118,7 @@
done();
}, 200);

suggestions._input.fire('focus', {});
suggestions.dispatchEvent(new CustomEvent('focus', { bubbles: true, composed: true }));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this was a mistake that also halted the tests. Added it back.

@dotpointer
Update to Polymer 2.x - part 3.
aa78196 
* Review changes.
* Adjusting dispatchEvent.
* Using Polymers Async.

Refs #28
@dotpointer
Copy link
Contributor Author

dotpointer commented Nov 15, 2018 •
edited
Loading

@Wurper Do you know if runhas the duration in the second parameter?

Polymer.Async.microTask.run(() => this.someMethod() /*, <timeout here?*/);

It seemed to pass the tests, but not sure if it really passes the timeout.

I cannot check because the documentation refuses to load.

fail

@Wurper
Copy link
Contributor

Wurper commented Nov 16, 2018

@dotpointer
According to https://polymer-library.polymer-project.org/2.0/docs/upgrade
you should use native setTimeout if a delay is used.

I checked the polymer source (if I looked at the correct one) and it looks like it puts in a microtask queue. No consideration to any delay.
https://github.com/Polymer/polymer/blob/665901ab2483385cf6ae7037663ad0f992e9b930/lib/utils/async.js

@dotpointer
Update to Polymer 2.x - part 3.
5451af2 
* Review fixes.
* Replacing Polymer.Async with window.setTimeout because the latter is missing a duration parameter.

Refs #28
@dotpointer
Copy link
Contributor Author

dotpointer commented Nov 16, 2018 •
edited
Loading

@Wurper Replacing Polymer.Async with window.setTimeout for the location which needed duration specification, pushed the change.

Wurper
Wurper approved these changes Nov 16, 2018
View reviewed changes
paper-autocomplete-chips.html Outdated
@@ -389,9 +387,8 @@
return;
}
this.push('selectedItems', newSelection);
this.async(function () {
Polymer.Async.microTask.run(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change to arrow function and skip bind(this)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, pushed a change.

@dotpointer
Update to Polymer 2.x - part 3.
43d163c 
* Review fixes.
* Using arrow functions.

Refs #28
@Wurper Wurper merged commit 37d1b22 into master Nov 16, 2018
@Wurper Wurper deleted the gh28-3 branch November 16, 2018 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wurper Wurper Wurper approved these changes

Assignees

@dotpointer dotpointer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@dotpointer @Wurper