-
Notifications
You must be signed in to change notification settings - Fork 62
[BA-3103] Utility for creating a link (deeplink, universal link, standard link) with a prolink #175
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
Conversation
✅ Heimdall Review Status
|
|
feedback:
|
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.
Could you post a screenshot of how the playground looks with these changes?
Looking great, couple comments to consider
| * Create a link with an encoded prolink query parameter and additional query parameters | ||
| * | ||
| * @param prolink - Base64url-encoded prolink payload | ||
| * @param baseUrl - Base URL to use for the link, defaults to https://base.app/base-pay |
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.
'baseUrl' is proper but feels overloaded because we are 'base' - is there a closeby synonym we can use like 'prefixUrl'?
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.
maybe just url? its scoped to the function so likely need a prefix
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.
updated to url
| * @param prolink - Base64url-encoded prolink payload | ||
| * @param baseUrl - Base URL to use for the link, defaults to https://base.app/base-pay | ||
| * @param additionalQueryParams - Additional query parameters to add to the link | ||
| * @returns { link: string } - Object containing the full link |
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.
preference for returning the string directly - let me know if you have thoughts in the other direction
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.
thanks for the reminder here, updated to return a string
| * @param prolink - Base64url-encoded prolink payload | ||
| * @param url - URL to use for the link, defaults to https://base.app/base-pay | ||
| * @param additionalQueryParams - Additional query parameters to add to the link | ||
| * @returns { link: string } - Object containing the full link |
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.
nit: update the docstring and usage example
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.
ugh I'm sorry for the oversight. thank you for catching 🙇
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.
LGTM!
Summary
prolinkToUniversalLinkprolinkandenvironmentHow did you test your changes?
When a prolink is generated on playground...

...a corresponding "link with prolink" result is created (defaulted to base-app link).

We also demonstrate the ability to customize the link URL in a top-level "Link with Prolink" tab:
