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

[EP-2362] Deduplicate sign in modals #1132

Merged
merged 6 commits into from
Jan 13, 2025
Merged

Conversation

canac
Copy link
Contributor

@canac canac commented Jan 10, 2025

Previously, the sign in modal could be opened from the session modal directly or from the register account modal inside of the session modal. The main difference was that the direct sign in modal didn't have the "Welcome back!" header/sidebar.

This PR removes the sign in modal from the session modal so that it will always be displayed inside of the register account modal. I added the minimal binding that when set to true hides the Welcome back header/sidebar.

I also cleaned up the bindings of signInModal so that no longer directly changes the state of the parent modal.

@canac canac requested a review from dr-bizz January 10, 2025 18:36
@dr-bizz
Copy link
Contributor

dr-bizz commented Jan 10, 2025

We need to look at the sign in link on AEM to see if we need to change it with this change.

@dr-bizz
Copy link
Contributor

dr-bizz commented Jan 10, 2025

We need to look at the sign in link on AEM to see if we need to change it with this change.

As I was reading your code, I see that we won't need to as you have kept the signin function on sessionModal.

@canac
Copy link
Contributor Author

canac commented Jan 13, 2025

@dr-bizz I found some places in the designation account editor and session enforcer that explicitly open the sign-in modal, so I fixed those as well.

@canac canac requested a review from dr-bizz January 13, 2025 17:22
src/common/services/session/sessionModal.component.js Outdated Show resolved Hide resolved
Comment on lines +101 to 104
modal = sessionModalService.open('register-account', {
backdrop: 'static',
keyboard: false
}).result
Copy link
Contributor

Choose a reason for hiding this comment

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

Should const modalType = find(enforced, { mode: EnforcerModes.donor }) this define if the register-account modal is minimal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you're asking. But now the register-account modal now defaults to minimal mode (welcomeBack is falsy), if that's what you mean.

@canac canac force-pushed the 2362-okta-combine-modals branch from 937f374 to 376e5b4 Compare January 13, 2025 19:22
@canac canac requested a review from dr-bizz January 13, 2025 19:35
Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

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

Great work!

@canac canac force-pushed the 2362-okta-combine-modals branch from 2b36860 to 203c89c Compare January 13, 2025 21:58
@canac canac merged commit ebc0f79 into 2362-okta Jan 13, 2025
1 check passed
@canac canac deleted the 2362-okta-combine-modals branch January 13, 2025 21:59
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