Skip to content

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.

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.
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.

this name leaves room for a JS SDK supported onLoad event in the future
@mewelling mewelling requested review from agillaspie and brdly-smith and removed request for mewelling October 10, 2024 18:34
Copy link
Contributor

@brdly-smith brdly-smith left a comment

Choose a reason for hiding this comment

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

LGTM, do we still need this @jasonabrown ?

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.

4 participants