-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Add loader and error handling to MFA modal #21089
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: nielslyngsoe <[email protected]>
Co-authored-by: nielslyngsoe <[email protected]>
| return html` | ||
| <umb-body-layout headline="${this.localize.term('member_2fa')}"> | ||
| <div id="main"> | ||
| ${when( |
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.
@copilot I would suggest this part being taking into its own render method. Then you can return if its loading, and return if its in error state. That would be easier to read and maintaine.
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.
Refactored into #renderContent() method using early returns instead of nested when() calls. This makes the code cleaner and easier to maintain. See commit 3c5f02f.
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.
Pull request overview
This PR adds loading state and error handling to the Current User MFA Modal, addressing the issue where users had no visual feedback during provider fetching. The implementation follows established patterns in the Umbraco codebase.
Key Changes:
- Added
_isLoadingand_errorstate management to provide user feedback during asynchronous operations - Implemented
<uui-loader>component display during provider loading - Added error handling with localized error messages using
errors_defaultErrorkey - Updated render logic with nested
when()directives for conditional UI display
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Umbraco.Web.UI.Login/package-lock.json | Peer dependency reorganization - multiple packages moved from regular to peer dependencies |
| src/Umbraco.Web.UI.Client/src/packages/user/current-user/modals/current-user-mfa/current-user-mfa-modal.element.ts | Added loading state, error handling, and conditional rendering for MFA provider modal |
Files not reviewed (1)
- src/Umbraco.Web.UI.Login/package-lock.json: Language not supported
Co-authored-by: nielslyngsoe <[email protected]>
Prerequisites
Description
The Current User MFA Modal (
current-user-mfa-modal.element.ts) fetches MFA providers asynchronously but provided no visual feedback during loading, leaving users staring at an empty modal with no indication that data is being fetched.Changes:
_isLoadingstate (defaulttrue) that displays<uui-loader>during provider fetch_errorstate that captures server errors and exceptions, displaying localized error message viaerrors_defaultErrorkey#renderContent()method using early returns for improved readability and maintainabilityTesting:
<uui-loader>displays while providers loadThe implementation follows existing patterns from
upgrader-view.element.tsand uses Lit's@state()decorator for reactive updates. The render logic uses early returns in a dedicated method instead of nested conditionals for better code maintainability.Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.