Navigation Menu

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

Bind this to custom attribute converter methods #3120

Merged

Conversation

nebarf
Copy link
Contributor

@nebarf nebarf commented Jul 7, 2022

Fixes #3108

@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2022

🦋 Changeset detected

Latest commit: b3ab89f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@lit/reactive-element Patch
lit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-cla
Copy link

google-cla bot commented Jul 7, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@nebarf nebarf force-pushed the reactive-element__converter-methods-binding branch from b5febc4 to b015dcf Compare July 7, 2022 06:50
Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @nebarf !

I made some comments around creating fewer short-lived function objects due to Function.bind() when setting properties/attributes.

A bigger change that we should consider outside of this fix (I think) would be to do the toAttribute/fromAttribute determination in createProperty() so it's done once per class, not on every property set. cc @sorvell

packages/reactive-element/src/reactive-element.ts Outdated Show resolved Hide resolved
packages/reactive-element/src/reactive-element.ts Outdated Show resolved Hide resolved
packages/reactive-element/src/reactive-element.ts Outdated Show resolved Hide resolved
… reflected property with a custom converter is set
@justinfagnani
Copy link
Collaborator

Thanks for the additional review @augustjk

@nebarf with those comments addressed this looks great!

@nebarf nebarf requested a review from augustjk August 5, 2022 16:50
Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! I just went ahead and combined the changeset files together which is our preferred way.

Copy link
Collaborator

@justinfagnani justinfagnani left a comment

Choose a reason for hiding this comment

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

LGTM!

@augustjk augustjk merged commit 6361a4b into lit:main Aug 5, 2022
@lit-robot lit-robot mentioned this pull request Aug 11, 2022
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.

[reactive-element] ComplexAttributeConverter, this is undefined in fromAttribute method
3 participants