-
Notifications
You must be signed in to change notification settings - Fork 2
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
FFS-2337: Multi-Provider Support in Javascript Front-end code #444
Conversation
…337/generic-provider-triggers
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.
Sorry, I have more comments coming, just haven't had time to dive in
…. Adds latency to button click, but reduced up front grabbing of token keys.
…om/DSACMS/iv-cbv-payroll into ffs-2337/generic-provider-triggers
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.
There's a few optimizations that we can make as well as resolve some things like duplicate imports and proper TS configuration. We should definitely do away with using the any
type.
…hrough into token api request
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 didn't review the tests very thoroughly, and may have missed something since I'm not as experienced at JS setup, but, with those caveats, it looks good to me. No blockers, but some suggestions to think about.
Do we want to run the tests automatically in CI? (I think we probably should!) Perhaps we can make a follow-up ticket for that.
|
||
export const createModalAdapter = (providerName: string) => { | ||
switch (providerName) { | ||
default: |
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 we make this "pinwheel" instead of default
? And raise some kind of JS error if the providerName
is unsupported?
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 don't see any other error throws in the JS code. What should the default behavior be? Redirect to /500.html - and will we have language specific implementations of this? Does something need to be logged to the track user action endpoint?
I wanted to keep Pinwheel as default until Argyle is added and then introduce the complexity.
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.
Not sure the best error behavior, since we don't have any frontend error-tracking at all right now. new Error(...)
would be fine with me for now, but I'm not sure whether there's a better pattern.
Ticket
Resolves FFS-2337.
Changes
Context for reviewers
Acceptance testing
:alert: Deploy block! @ffs-eng I just merged PR [#123] and will be doing acceptance testing in demo - please don't deploy until I'm finished!
)