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.

Sign up for GitHub

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

Jump to bottom

Add loadEventCallback #90

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add loadEventCallback #90

wants to merge 3 commits into from
+18 −1

Conversation

jasonabrown
Copy link
Contributor

The JS SDK allows for a callback on certain methods. Since the React SDK handles _ps("load") under the hood we need allow users to specify an error handling callback for this method.

Verified locally.

@jasonabrown
Add loadEventCallback
cd3bc30 
The snippet allows for a callback on certain methods. Since the React SDK handles _ps("load") under the hood we need allow users to specify an error handling callback for this method.
cdhcs1516
cdhcs1516 approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@cdhcs1516 cdhcs1516 left a comment

Choose a reason for hiding this comment

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

Can you check the indents? They look abnormal.

@cdhcs1516
Copy link
Contributor

@jasonabrown and do we want to add this prop to the documentation?

@jasonabrown
Copy link
Contributor Author

@cdhcs1516 thank you for the quick review! Curious your thoughts on naming convention for this prop. We could alternatively call it onLoad, but clarify in the documentation that it is not included in onAll.

@cdhcs1516
Copy link
Contributor

@cdhcs1516 thank you for the quick review! Curious your thoughts on naming convention for this prop. We could alternatively call it onLoad, but clarify in the documentation that it is not included in onAll.

onLoad sounds good. But why is it not included in onAll? This function always gets called during _ps('load'), right?

@jasonabrown
Copy link
Contributor Author

@cdhcs1516 the underlying JS SDK does not have a trigger event for load so we instead rely on the event_callback argument.

@jasonabrown
Updated naming convention to onLoadCallback
edb32e7 
this name leaves room for a JS SDK supported onLoad event in the future
@jasonabrown
Update README.md
55216c1
@mewelling mewelling requested review from agillaspie and brdly-smith and removed request for mewelling October 10, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdhcs1516 cdhcs1516 cdhcs1516 approved these changes

@agillaspie agillaspie Awaiting requested review from agillaspie

@brdly-smith brdly-smith Awaiting requested review from brdly-smith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jasonabrown @cdhcs1516