-
Notifications
You must be signed in to change notification settings - Fork 320
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
G2: Trigger removeLoading on language load finish #3730
base: 7.24
Are you sure you want to change the base?
Conversation
@@ -154,9 +154,9 @@ export default class Header extends View { | |||
// Order of these calls is important for - | |||
// loader --> security/factor beacon swap. | |||
removeBeacon(this); | |||
this.$el.removeClass(NO_BEACON_CLS); |
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.
Need to remove class NO_BEACON_CLS
anyway.
Otherwise beacon can have incorrect height after transition loading -> null -> constroller beacon
(see videos)
In case of this transition isLoading
(line 157) will be false in swap
callback, but before Animations.swapBeacons
it was true.
So to fix this height issue we can calculate isLoading
before swap animation or remove NO_BEACON_CLS
class anyway.
Example on /signin/verify/piv
Before this fix:
without.wasLoading.mov
After:
with.wasLoading.mov
@@ -185,6 +185,10 @@ export default class Header extends View { | |||
|
|||
// Hide the beacon on primary auth failure. On primary auth success, setBeacon does this job. | |||
removeLoadingBeacon() { | |||
if (!isLoadingBeacon(this.currentBeacon)) { |
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.
Do not remove non-loading beacon
appState.trigger('loading', false); | ||
appState.trigger('removeLoading'); |
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.
trigger('loading', false)
will do nothing here because of this line:
okta-signin-widget/src/v1/views/shared/Header.ts
Lines 180 to 181 in 97e8b74
if (!isLoading || isLoadingBeacon(this.currentBeacon)) { | |
return; |
We need to trigger removeLoading
event. There are similar approaches in v1 codebase. Example
okta-signin-widget/src/v1/controllers/RecoveryLoadingController.js
Lines 31 to 33 in 97e8b74
.catch(function() { | |
self.options.appState.trigger('loading', false); | |
self.options.appState.trigger('removeLoading'); |
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.
LGTM. Please target 7.24
before merging.
a9c5ff2
to
9742995
Compare
Description:
To reproduce:
language: 'uk'
(or any other supported language) in.widgetrc.js
yarn start --watch
localStorage.removeItem('osw.languages');
at the beginning ofplayground/main.ts
to load language every time200
to0
here:okta-signin-widget/src/util/LanguageUtil.js
Line 14 in 188eaa6
PR Checklist
Issue:
Reviewers:
Screenshot/Video:
Before :
After :
Downstream Monolith Build:
https://bacon-go.aue1e.saasure.net/commits?artifact=monolith&page=1&pageSize=6&sha=dcf9166f17fd4a6fcc2909d4c46d776e1e761156&tab=main