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

[composer] Made the value prop reactive. #93

Merged
merged 3 commits into from
Oct 4, 2021

Conversation

AaronDDM
Copy link
Contributor

@AaronDDM AaronDDM commented Sep 28, 2021

  • Previously the value prop could only be set at instantiation of the composer component. This prevented users from being able to change its value dynamically via JS like element.value = { ...newValues }.
  • Un-supported BC break: you can no longer set element.value.property = change and expect any reactive changes. You must set a new object e.g element.value = { property: change } to trigger Svelte's reactive re-render. Tests have been updated to reflect this.

License

I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@AaronDDM AaronDDM requested a review from a team September 28, 2021 16:29
@AaronDDM AaronDDM self-assigned this Sep 28, 2021
@AaronDDM AaronDDM requested review from ozsivanov and heatlikeheatwave and removed request for a team September 28, 2021 16:29
@vercel
Copy link

vercel bot commented Sep 28, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/nylas/components/2w7oXy8uMDsxV5auB9dHuDYiNhk8
✅ Preview: https://components-git-adm-sc-66489make-value-prop-reactive-nylas.vercel.app

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #66489: [components-composer] Make the value prop reactive.

Copy link
Contributor

@ozsivanov ozsivanov left a comment

Choose a reason for hiding this comment

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

I guess ideally we'd avoid having value be an object, but this looks good for now!

…y, so that is no longer supported. Pass a new object!
Comment on lines +165 to +168
$: if (value) {
mergeMessage(value);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the type of value?

Copy link
Contributor Author

@AaronDDM AaronDDM Oct 1, 2021

Choose a reason for hiding this comment

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

Message | void but I suppose it could be undefined as well...

@AaronDDM AaronDDM merged commit 7fe7f2b into main Oct 4, 2021
@AaronDDM AaronDDM deleted the adm/sc-66489/make-value-prop-reactive branch October 4, 2021 18:49
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

3 participants