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

bugfix for #42 #48

Merged
merged 3 commits into from
Jun 8, 2019
Merged

bugfix for #42 #48

merged 3 commits into from
Jun 8, 2019

Conversation

vcolavin
Copy link
Contributor

@vcolavin vcolavin commented Jun 7, 2019

Check out #42 for a description. Basically, if you passed in an input with a name or value, those attributes would be trampled by custom component.

I used a mutation observer as suggested by @patrickarlt to watch for changes on the child element. The custom component and child input now stay in sync.

I tested that this works, but I don't have an automated test for it. If that is a requirement please let me know and I'll write one!

@vcolavin vcolavin added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jun 7, 2019
@@ -76,8 +76,84 @@ export namespace Components {
}
}

declare global {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why stencil decided to make all these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happened when I ran npm test 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine. Stencil automatically generates this file for typings while you are developing. I'm not 100% sure if we can ignore it but it feels safer to leave it in.

Copy link

@stevenalbers stevenalbers left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm going to merge it. This also appears to handle the following cases like:

<calcite-switch name="baz" value="woo">
  <input id="switch-2" type="checkbox" checked name="foo" value="bar">
</calcite-switch>

So only the things on <input> "win" and get synced up to the component.

}

private syncProxyInput() {
private syncThisToProxyInput = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private syncThisToProxyInput = () => {
function syncFromProxyInput() {

Perhaps this? It feels both more readable as a class method rather then a property that is an arrow function and the function name seems more understandable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind I totally just realized why you are using an arrow function here because now you don't have to bind the callback.

I do really thing syncThisToProxyInput is confusing. I like syncFromProxyInput and syncToProxyInput better.

this.value = this.inputProxy.value;
};

private syncProxyInputToThis = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps in the same vein as my above comments I think this should be syncToProxyInput. The renames mean you end up writing code like this.syncToProxyInput() and this.syncfromProxyInput.

@patrickarlt patrickarlt merged commit 1212355 into master Jun 8, 2019
@vcolavin vcolavin deleted the vin10439/bugfix/#42 branch June 27, 2019 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants