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

Add custom 2FA based on default codeigniter4/shield email 2FA #416

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

kgrruz
Copy link
Contributor

@kgrruz kgrruz commented Feb 16, 2024

Add custom 2FA based on default codeigniter4/shield email 2FA. Just to make use of the themes.

Copy link
Owner

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

Thanks for this! Definitely a missed piece.

I see you're extending the action from Shieid - that's perfect. Because of that please delete an methods in this class that don' specifically need to be here for theming purposes.

Actually, IIRC the Actions use a Themeable trait that only provides $this->view() method. Would it be possible to ONLY override that method in this action to make use of the theme? Then all of the other methods wouldn't have to be duplicated which would make it safer then us having to constantly update those files when the logic changes?

@kgrruz kgrruz force-pushed the add-2FA-shield-action branch from 862bdc0 to 30dbcc0 Compare February 16, 2024 16:32
@kgrruz
Copy link
Contributor Author

kgrruz commented Feb 16, 2024

Done. Thank you for the explanation. If any other changes are needed, just tell me.

@kgrruz kgrruz force-pushed the add-2FA-shield-action branch 2 times, most recently from e2cc9f9 to a86a261 Compare February 20, 2024 23:59
@kgrruz kgrruz force-pushed the add-2FA-shield-action branch from a86a261 to 5421328 Compare February 21, 2024 00:46
@kgrruz kgrruz requested a review from lonnieezell February 21, 2024 00:48
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.

2 participants