Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add input_amount to events #451
Add input_amount to events #451
Changes from 2 commits
3c5dbae
977f579
af08900
7c32c67
ae6ac85
b78eea7
be59e14
6c4efe2
515ddc7
80262d5
66b1eed
02eb487
9ef4bde
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Any reason we are using a ref, and especially using it only here? Alternatively, we could change the
onChange
prop of theAssetNumericInput
component and pass teh new value in it. So you could access it right away. Here, you would then be able to doThere 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.
Well, the idea of the ref (and timeout) is because the
onChange
will always be triggered right on first modification. We need to wait, somehow, for the actual stable amount.If we just send the event on change, the value registered will be always
0
or whatever default value is on the form as soon as the user started typing.I think we will have the same issue on the
AssetNumericInput
. The other alternative I can think of, is to track it when the output is stable, here. Seems more cumbersome.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.
So the ref is just used in order not to track the first (prefilled) amount?
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.
Yes, it is to store the value. I tried using the "current"
fromAmount
value at the time of the firstonChange
, but the timeout function will take the value as soon as it is triggered (the initial one), which will be again the default.Since the ref updates from the
useEffect
above, at the moment of triggering the event, it will read the last value typed.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 pushed a new commit with a suggested change. I thought it might be easier to show an alternative approach instead of describing it. What do you think about using the ref only for tracking if the user touched the field but then debounce the actual value to avoid tracking too many amounts?
I also learned that useDeferredValue is not necessarily suited to debounce values like for user amounts as the timeout is not configurable.
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 saw you replaced the ref and timeout with a debounced value. Did it worked? Because I think this would send the current
debouncedValue
at the moment of the firstonChange
triggering.Because here, since
fromAmountFieldTouched
is a dependency, it would execute the effect right away.Yes useDeferred was a loose import.
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 fixed that by adding another statement to the effect hook. I also removed the
amount_type
event from the list of unique events. Everything seems to work as expected now.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.
So we will capture all user quotes? Makes sense, only we will be flooded by events. But I suppose GA can handle such things.
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 personally understood that the valuable outcome of this change is that marketing is able to see all amounts that the user tried. If it's just about the final amount, we can check the input amount on the other events after all. So yes, 'flooding' would be intended I think.
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.
All good then!
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.
@pendulum-chain/devs let me know if you like the addition of this state, if you prefer it to be more minimal (only
fromAmount
) or to avoid this altogether.Although it adds extra code, I found it less confusing than passing
fromAmount
to the event context.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'm fine with keeping it the way you implemented it 👍