-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: widget events for send to wallet #309
Conversation
Hey! This is your new endopint: https://10069250.widget-widgeteven.pages.dev |
Hey! This is your new endopint: https://04c1b7ee.widget-widgeteven.pages.dev |
Hey! This is your new endopint: https://733c2353.widget-widgeteven.pages.dev |
} | ||
|
||
const fieldValueToEmittedEvents: FieldValueToEmittedEvents = { | ||
toAddress: (address, emitter) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add all fields here to avoid adding them in the future anyway. We should also think about having one event for all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go with one event for them all. It's simpler that way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gonna add in better type inference too
packages/widget/src/types/events.ts
Outdated
@@ -19,6 +19,7 @@ export enum WidgetEvent { | |||
WalletConnected = 'walletConnected', | |||
WidgetExpanded = 'widgetExpanded', | |||
PageEntered = 'pageEntered', | |||
SendToWalletAddressChanged = 'sendToWalletAddressChanged', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add all fields, how about we come up with one event that handles all the fields with fieldName, oldValue and newValue.
Maybe name it something like
- FormFieldChanged
- FormValueChanged
Basically, the idea is to subscribe to the form changes, rather than individual events for every field. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, when doing this I did wonder it we would end you wanting to do it for all values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have gone with this approach using FormFieldChanged, I'm gonna look at better type inference as well. Also I think I used value rather than newValue so will change that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've improved the types now as well
emitEventForFieldValueChange( | ||
fieldName, | ||
value, | ||
actions.getFieldValues(fieldName)[0], | ||
emitter, | ||
); | ||
|
||
actions.setFieldValue(fieldName, value, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set value first and, after that fire an event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actions.getFieldValues will not have the old value to do the comparison to check for change if you call actions.setFieldValue first. I tried it the other way around initially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have changed this
(Object.keys(formValues) as FormFieldNames[]).forEach((fieldName) => { | ||
emitEventForFieldValueChange( | ||
fieldName, | ||
formValues[fieldName], | ||
actions.getFieldValues(fieldName)[0], | ||
emitter, | ||
); | ||
}); | ||
|
||
actions.setUserAndDefaultValues(formValues); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we set value first and, after that fire an event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actions.getFieldValues will not have the old value to do the comparison to check for change if you call actions.setFieldValue first. I tried it the other way around initially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can save it to a variable like
const oldValue = actions.getFieldValues(fieldName)[0]
and then execute after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would actually become a bit more complicated in the example above for the setUserAndDefaultValuesWithEmittedEvents
- you would have to store the oldValues in another object map first and then once setUserAndDefaultValues
is executed you would have to the iterate over the old values map.
I don't see what the benefit will be of changing the order of execution there and I think it will make the code a little less cleaner - but I do it and can decide if you are happier with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have put something in place for this
Hey! This is your new endopint: https://1ab1589f.widget-widgeteven.pages.dev |
Hey! This is your new endopint: https://530a5df8.widget-widgeteven.pages.dev |
Hey! This is your new endopint: https://11cad9c0.widget-widgeteven.pages.dev |
Hey! This is your new endopint: https://24af08e5.widget-widgeteven.pages.dev |
Jira: LF-8931
This PR adds a "FormFieldChanged" to the widget. This event will fire when..
Note: that with this implementation the initialise value from the widget config will not emit an event.
To catch the event specifically for the send to wallet address change you will need to check the fieldName value in on the payload
The event payload will looks like this
Widget Events Tooling
I've also added some basic tooling to the the playground to assist with testing and working with events. This should help with the other events we need to add to widget as well. This sits behind the dev view toogle.
To view the tool