-
Notifications
You must be signed in to change notification settings - Fork 321
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
Gen3: Support afterTransform
hook
#3667
base: master
Are you sure you want to change the base?
Conversation
cc0c55b
to
560b8a8
Compare
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.
PS: Question: do we still need these types? https://github.com/okta/okta-signin-widget/blob/master/src/v3/src/types/widget.ts#L136-L137
do you want to remove them or update them?
It looks like these 2 types are not used and this confused me, so maybe comment them or remove? |
src/v3/src/types/hooks.ts
Outdated
idxContext?: IdxContext; | ||
} | ||
|
||
export type TransformHookFunction = (formBag: FormBag, context: TransformHookContext) => void; |
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.
Order of params is inconsistent with the existing event hooks
https://github.com/okta/okta-signin-widget#events
signIn.on('ready', function (context) {/* ... */})
signIn.on('afterError', function (context, error) {/* ... */})
signIn.on('afterRender', function (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.
What if we merge context and formBag in one first argument?
signIn.on('afterTransform', function (context) {/* ... */})
context = {
formBag,
// context used for existing event hooks
formName, authenticatorKey, methodType,
// additional context
userInfo, deviceEnrollment, nextStep, idxContext,
}
); | ||
if (customBoolIndex != -1) { | ||
const customBool = formBag.uischema.elements[customBoolIndex] as FieldElement; | ||
customBool.translations.find(t => t.name === 'label').value = loc('custom.field.terms.label'); |
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.
Exposed loc
util for afterTransform
hook and used in this sample to demonstrate i18n capabilities.
Translations are performed before afterTransform
hook, so setting i18nKey
in items of translations
array would have no effect.
Thoughts?
@shawyu-okta mind taking a look? I've reviewed, but need to talk to docs team about the README updates |
playground/package.json
Outdated
"main": "main.ts" | ||
"main": "main.ts", | ||
"scripts": { | ||
"generate:hooks:js": "../node_modules/.bin/tsc -p ./hooks/tsconfig.json && cp -r ./hooks/js/playground/hooks/pages ./hooks && rm -rf ./hooks/js" |
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: would adding "typescript": "*"
and moving tsconfig to this dir simplify this?
Maybe the playground could use some more typing?
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.
Moved tsconfig.json
to playground
dir
. wip .
c63c014
to
aa53676
Compare
Description:
Readme: link
Example of usage: see hooks dir
Example of style overrides: see customize.css
Hook examples are present in 2 versions: TS (for playground) and JS (to copy-paste into code editor in Okta admin).
Script to convert hooks example TS -> JS:
yarn --cwd playground generate:hooks:js
PS: Question: do we still need these types? https://github.com/okta/okta-signin-widget/blob/master/src/v3/src/types/widget.ts#L136-L137
PR Checklist
Issue:
Reviewers:
Screenshot/Video:
Downstream Monolith Build: