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

Admin settings Key metrics answer preview loading issue. #6428

Closed
mohitwp opened this issue Jan 18, 2023 · 19 comments
Closed

Admin settings Key metrics answer preview loading issue. #6428

mohitwp opened this issue Jan 18, 2023 · 19 comments
Labels
Exp: SP Good First Issue Good first issue for new engineers P1 Medium priority Type: Bug Something isn't working

Comments

@mohitwp
Copy link
Collaborator

mohitwp commented Jan 18, 2023

Bug Description

If analytics is connected then for milliseconds "CTA gets load" before we see the answer preview screen.

Steps to reproduce

  1. Provide answers to User Input Questions.
  2. Set up Analytics.
  3. Go to Admin settings.
  4. Notice before Question and Answers preview 'Key metrics CTA' gets load for milliseconds.

Screenshots

Recording.211.mp4

Additional Context

  • PHP Version:
  • OS: [e.g. iOS]
  • Browser: [e.g. chrome, safari]
  • Plugin Version: [e.g. 22]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • There should be no flicker that shows User Input Admin CTA in the Key Metrics section of Settings page.
    • Note that is is very hard to spot, throttle connection speed to observe it.

Implementation Brief

  • The flickers appears to be happening due to isUserInputCompleted briefly returning undefined before resolving while the other prerequisites are resolved and met. (ie. Both Analytics and SC is connected and has data).
  • To account for this, return null from the UserInputSettings component when isUserInputCompleted is undefined or 'true'

Test Coverage

QA Brief

  • Provide answers to User Input Questions.
  • Set up Analytics.
  • Go to Admin settings.
  • Flicker should be eliminated

Changelog entry

  • Fix key metrics preview loading issue on the admin settings page.
@mohitwp mohitwp changed the title Admin settings Key metrics settings loading issue. Admin settings Key metrics answer preview loading issue. Jan 18, 2023
@kuasha420 kuasha420 added Type: Bug Something isn't working P1 Medium priority Good First Issue Good first issue for new engineers Exp: SP labels Jan 23, 2023
@kuasha420 kuasha420 removed their assignment Jan 23, 2023
@eugene-manuilov eugene-manuilov self-assigned this Jan 23, 2023
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Jan 23, 2023
@aaemnnosttv
Copy link
Collaborator

@kuasha420 @eugene-manuilov userInputState was recently removed / repurposed into isUserInputCompleted so this my either no longer be an issue or at least need to be updated.

@kuasha420
Copy link
Contributor

@aaemnnosttv Can you link me to the issue where this happened? I assume it hasn't been merged into develop yet?

@kuasha420
Copy link
Contributor

Nevermind, found the issue #5900 and PR #6119. I'll wait for #6119 to be merged before having another look at this, I'll keep this one with me until then. Cheers

@kuasha420
Copy link
Contributor

@aaemnnosttv Checked and confirmed the issue still exists with the latest develop and updated the IB accordingly. Moving straight to EB.

@kuasha420 kuasha420 removed their assignment Jan 27, 2023
@sashadoes sashadoes self-assigned this Jan 28, 2023
@sashadoes sashadoes removed their assignment Jan 30, 2023
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jan 30, 2023
@mohitwp mohitwp self-assigned this Feb 1, 2023
@mohitwp
Copy link
Collaborator Author

mohitwp commented Feb 1, 2023

QA Update ❌

@sashadoes

  • Tested on dev environment.

  • When user navigates to admin settings, then I can notice flicker just before key metrics answer preview. Now CTA is not showing, but it loads plugin status first then display key metrics settings. You can clearly see this in below video. Pause video at 0.04 seconds and forward it manually.

Recording.218.mp4

@mohitwp mohitwp assigned sashadoes and unassigned mohitwp Feb 1, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Feb 1, 2023

@mohitwp good spot. I spotted the same when I was testing the key metrics banner on the main dashboard. I was informed by Jimmy that this would be fixed in another ticket. You can read about that here. It looks like 6209 and 6210 would fix this, but I will let @sashadoes confirm that for admin settings.

@sashadoes
Copy link
Collaborator

Hi @mohitwp. After the check, I can confirm that the implementation I made is not connected to the above issue in Account Settings. So should be fixed as @wpdarren stated.

@wpdarren
Copy link
Collaborator

wpdarren commented Feb 1, 2023

@jimmymadon Hi, the issue I found with the flicker of the key metrics banner on the dashboard is also appearing on the admin settings screen. Will the tickets you mentioned here also fix this? I don't want us to approve this and then find out it'll not be fixed. Thank you! c.c. @mohitwp

@jimmymadon
Copy link
Collaborator

@wpdarren I believe the new settings panel is "new code" already - am I right @sashadoes / @kuasha420? So even though there are some Key Metric issues that enhance this settings panel, we should fix this flickering as part of this issue preferably in my opinion.

@mohitwp
Copy link
Collaborator Author

mohitwp commented Feb 7, 2023

QA Update ❌

@sashadoes

-Tested on dev environment.

  • I've noticed that when we navigate to settings page then switch to admin settings then loader is showing and flicker is not happening. But if we refresh the page, then we can see notice existing flicker issue. Pause attached videos and manually forward them to see the issue.
Recording.223.mp4
Recording.221.mp4

@mohitwp mohitwp assigned sashadoes and unassigned mohitwp Feb 7, 2023
@sashadoes
Copy link
Collaborator

Hi @mohitwp, thanks for the update.
From what I can see from the code it is not a flicker, It is just how the User Preview block is designed.
On the page load User Preview waits for settings data to be loaded and shows a progress bar meanwhile.
If settings load fast you can experience a fast change from the progress bar to the User Preview block which may look like a flicker.

So..How do you want it to behave? Do you want to remove the progress bar or maybe add some delay so that rapid change will never happen? cc @wpdarren @jimmymadon

@mohitwp
Copy link
Collaborator Author

mohitwp commented Feb 7, 2023

@sashadoes It would be good if we can add progress bar also when user refreshes the page. Not only when user switch to admin settings tab. @jimmymadon @wpdarren Can you please suggest what ideal behavior we can achieve in this case ?

@wpdarren
Copy link
Collaborator

wpdarren commented Feb 7, 2023

@sashadoes I agree with @mohitwp it looks a odd UX/UI when the progress bar loads for a second then the answers to the questions load when you refresh the page. Especially because it doesn't occur when you click on the admin settings. Both should behave the same in my opinion. Interested in @jimmymadon opinion on this. It's minor, but will likely come up in the bug bash so better to resolve it now.

@jimmymadon
Copy link
Collaborator

@wpdarren @mohitwp

There are two cases here:

  1. If the User Input survey has been completed, the panel always shows a progress bar when loading (as @sashadoes mentions). Clicking on Admin Settings or refreshing the page has this same behaviour - refreshing is just quicker in the video probably due to cache. We probably have similar "super quick" loading bars in other components too. So not sure what the "correct" behaviour here should be. Perhaps this should be a separate issue.
  2. If the User input survey has not been completed, there is a prominent flicker in my testing. If the site is in gathering data state, the behaviour is worse - the component is loaded and then disappears completely. This flicker can easily be avoided by returning null if analyticsIsGatheringData or searchConsoleIsGatheringData are undefined. Not sure if this is worth creating a separate issue for - a follow up PR might be quick here.

c.c. @aaemnnosttv

@aaemnnosttv aaemnnosttv assigned jimmymadon and unassigned sashadoes Feb 8, 2023
@aaemnnosttv
Copy link
Collaborator

After discussing with @jimmymadon, we agreed to quickly fix the second case he mentioned above related to gathering state conditions.

We'll open a new issue about addressing flickers due to "super quick" progress/loaders more holistically 👍

@aaemnnosttv
Copy link
Collaborator

After some further investigation with @jimmymadon, we found that the conditional logic to display the user input CTA on the admin settings based on module gathering states was a bit premature and will inevitably lead to a flicker or shift any time we need to check if the modules are in a gathering state that requires an API request (regardless if we check for gathering or not gathering). We've gone ahead and removed that conditional logic for now, with the intention to reintroduce it in (or after) #5933 which ideally should have been done first as this will provide the gathering state on page load so long as we know that the module is not in a gathering state. So we'll follow up to reintroduce this after that issue.

Another reason for deferring this is related to a few existing tests which were passing because of the flicker (CTA seen as present) before disappearing because of the lack of gathering state being considered/provided in the test. This change brings the interface back into parity with the expectations of the test without requiring additional time-consuming changes to fix.

TL;DR there is no more flicker on the admin settings because the user input CTA should be shown regardless of the gathering state now, but this will be corrected in a future issue.

@aaemnnosttv
Copy link
Collaborator

@mohitwp please see the updates outlined above, otherwise this should be ready for another pass 👍

@aaemnnosttv aaemnnosttv assigned mohitwp and unassigned jimmymadon Feb 8, 2023
@mohitwp
Copy link
Collaborator Author

mohitwp commented Feb 9, 2023

QA Update ✅

  • Tested on main.
  • Verified that if site is in gathering state and analytics is connected, then CTA is showing under Admin settings page and on SK dashboard.
  • Verified that no flicker showing related to CTA under Admin settings.
Recording.224.mp4
Recording.225.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Good First Issue Good first issue for new engineers P1 Medium priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants