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

G2: Trigger removeLoading on language load finish #3730

Open
wants to merge 6 commits into
base: 7.24
Choose a base branch
from

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Oct 1, 2024

Description:

To reproduce:

  • add language: 'uk' (or any other supported language) in .widgetrc.js
  • run yarn start --watch
  • add localStorage.removeItem('osw.languages'); at the beginning of playground/main.ts to load language every time
  • change 200 to 0 here:

PR Checklist

Issue:

Reviewers:

Screenshot/Video:

Before :

Screenshot 2024-10-01 at 19 30 55

After :
Screenshot 2024-10-01 at 19 31 10

Downstream Monolith Build:

https://bacon-go.aue1e.saasure.net/commits?artifact=monolith&page=1&pageSize=6&sha=dcf9166f17fd4a6fcc2909d4c46d776e1e761156&tab=main

@@ -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);
Copy link
Contributor Author

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)) {
Copy link
Contributor Author

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

Comment on lines 21 to +22
appState.trigger('loading', false);
appState.trigger('removeLoading');
Copy link
Contributor Author

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:

if (!isLoading || isLoadingBeacon(this.currentBeacon)) {
return;

We need to trigger removeLoading event. There are similar approaches in v1 codebase. Example

.catch(function() {
self.options.appState.trigger('loading', false);
self.options.appState.trigger('removeLoading');

@denysoblohin-okta denysoblohin-okta changed the title G1/G2: Trigger removeLoading on language load finish G2: Trigger removeLoading on language load finish Oct 2, 2024
@denysoblohin-okta denysoblohin-okta marked this pull request as ready for review October 2, 2024 17:08
Copy link
Contributor

@shawyu-okta shawyu-okta left a 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.

@denysoblohin-okta denysoblohin-okta changed the base branch from master to 7.24 October 4, 2024 08:54
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