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.

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

feat: add login-config-totp.ftl page #127

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

aidangilmore
Copy link
Contributor

Adds login-config-totp.ftl page, resolving #102

@garronej
Copy link
Collaborator

Wonderfull, thank you very much, looks clean.

@aidangilmore
Copy link
Contributor Author

My pleasure! Let me know if you have any questions or need any changes.

@garronej
Copy link
Collaborator

garronej commented Jun 28, 2022

@aidangilmore,
Yes just one maybe, have you tested in Keycloak or just with yarn start?
Have you linked your version of Keycloakify in keycloakify-demo-app?
I'm asking because I just created the contributing doc, I would like to know how it went for you.

@garronej garronej merged commit fca6280 into keycloakify:main Jun 28, 2022
@garronej
Copy link
Collaborator

I released a new version including your changes.

@aidangilmore aidangilmore deleted the add-totp-support branch June 28, 2022 20:23
@aidangilmore
Copy link
Contributor Author

aidangilmore commented Jun 28, 2022

I tested using yarn start until I was satisfied that there weren't any major issues, then tested in Keycloak to make sure I didn't miss anything. All the testing was done using a linked Keycloakify as per the contribution instructions.

I didn't have any major issue following the contributing document, just a small one caused by the versions of React not lining up between Keycloakify devDependencies and keycloakify-demo-app. I'm not sure if that's just an issue with my machine for whatever reason or something reproducible. Regardless, I just bumped the version locally and everything worked fine.

It wasn't entirely clear how to get the types for KcContextBase, so I just looked at the Keycloak source code and used the types from there. If there is a better way of doing this, it may be worth mentioning in the doc.

I will say that I really liked that you pointed to a sample commit that I could base my changes off of.

Does that answer your questions?

@garronej
Copy link
Collaborator

Yes, thank you for the lovely feedback.

didn’t have any major issue following the contributing document, just a small one caused by the versions of React not
lining up between Keycloakify devDependencies and keycloakify-demo-app.

It makes sense that you had issues linking react, I forgot, Keycloakify is still using 17 when the demo app is using 18. I'll correct this.

It wasn't entirely clear how to get the types for KcContextBase, so I just looked at the Keycloak source code and used
The types from there. If there is a better way of doing this, it may be worth mentioning in the doc.

No, unfortunately there is no better way than to infer from usage just like you did.

I will say that I really liked that you pointed to a sample commit that I could base my changes off of.

OK good to know!

Thanks again for taking the time to contribute to Keycloakify!

garronej added a commit that referenced this pull request Jun 28, 2022
@garronej
Copy link
Collaborator

@all-contributors please add @aidangilmore for tests and code

@allcontributors
Copy link
Contributor

@garronej

I've put up a pull request to add @aidangilmore! 🎉

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.

None yet

2 participants