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

Hide widget tiles when Key Metrics aren't setup #7061

Closed
jimmymadon opened this issue May 18, 2023 · 9 comments
Closed

Hide widget tiles when Key Metrics aren't setup #7061

jimmymadon opened this issue May 18, 2023 · 9 comments
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@jimmymadon
Copy link
Collaborator

jimmymadon commented May 18, 2023

Feature Description

In #6261, we refactored the selectors so that getKeyMetrics within the core/widgets core/user store determines which key metric widget tiles should be displayed on the dashboard. This selector returns an empty array when the Key Metrics widget itself is not set up. Based on this functionality, we should hide the widget tile placeholders which are currently being rendered even when the user has not answered the questionnaire nor picked their own metrics (i.e the Key Metrics widget is not setup).


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

Acceptance criteria

  • The individual Key Metric widget tiles should not be rendered within the Key Metrics widget area when the logged-in user has not answered the questionnaire nor picked their own metrics, i.e. no tiles (their placeholders) should be displayed until the Key Metrics widget is set up.
  • Once a user answers the questionnaire or picks their own metrics, only those Key Metric tiles returned by the getKeyMetrics selector from core/widgets should be rendered. (This selector already returns the appropriate metrics set based on either the user input questionnaire answers or the user picking their own metrics.)
  • UPDATE: Furthermore, the Key Metrics widget area should not be rendered if the Key Metrics widget is setup but the "Hide key metrics toggle" in Admin Settings is turned on.

Implementation Brief

  • Within assets/js/googlesitekit/datastore/user/key-metrics.js:

    • Create a new selector, isKeyMetricActive which takes a key metric widgetSlug as a parameter. This selector should use the getKeyMetrics from the same file (core/user datastore) to check if this widgetSlug is present.
  • In all assets/js/modules/<module-name>/index.js files:

    • When registering any key metric widget using registerWidget, pass the above selector as a function to the isActive parameter.
  • In DashboardMainApp.js:

    • Use the isKeyMetricsWidgetHidden selector and iff it returns true, do not not render the <WidgetContextRenderer> for the Key Metrics Widget context.
  • Delete the assets/js/components/KeyMetrics/utils.js file and remove the whenKeyMetricsWidgetVisible HOC and its usage in existing widget tiles.

  • Within assets/js/components/KeyMetrics, create a new functional component called MetricTile:

  • Within all Key Metric widget components:

    • Instead of rendering the <MetricTile-xxxx-> component directly, wrap it with <MetricTile> created above. Pass the <WidgetNull> component as a prop to it. For components which have not been implemented as yet and have "TODO" in them, simply wrap the
      with the <MetricTile> component.
  • Move the keyMetricsWidgetHidden check from all individual components into the new <MetricTile> component. This way the logic to render <WidgetNull>, both, when Key Metrics aren't setup and when the Key Metrics widget is "hidden" (based on the settings toggle), will reside in a single component.

Test Coverage

  • Write a JS test for the <MetricTile> component and assert <WidgetNull> is rendered as per the IB above.

QA Brief

  • With the userInput feature flag disabled, smoke test the Site Kit dashboard and verify there are no console errors.
  • Enable the userInput feature flag for the following instructions.
  • After resetting site kit, verify the first point in the AC.
  • Now answer the questionnaire and verify the tiles are displayed as per the AC of Create data store infrastructure to fetch answer-based metrics #6234. Reset site kit or delete the googlesitekit_user_input_settings option from wp_options and repeat this step to check the appropriate tiles are displayed based on the answer to question 1 of the questionnaire.
  • Verify the Key Metrics widget area is displayed and hidden appropriately by switching the "Display Key metrics in dashboard" toggle within Admin Settings. Also test that the "Navigation chips" for Key Metrics is hidden, displayed and highlighted correctly depending on when Key Metrics are shown or hidden on the dashboard. When hidden, the navigation chips should work as before.

Changelog entry

  • Hide Key Metric widget tiles when the feature isn't setup.
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon May 18, 2023
@mxbclang mxbclang added P1 Medium priority Type: Enhancement Improvement of an existing feature labels May 22, 2023
@tofumatt
Copy link
Collaborator

ACs here are good, assigning it back to you @jimmymadon 👍🏻

@tofumatt tofumatt removed their assignment May 24, 2023
@jimmymadon jimmymadon removed their assignment Jun 6, 2023
@tofumatt tofumatt self-assigned this Jun 6, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Jun 6, 2023

I think the refactor here is definitely worth doing, I'm going to remove "potential" refactor from the IB.

IB ✅

@tofumatt tofumatt assigned jimmymadon and unassigned tofumatt Jun 6, 2023
@aaemnnosttv
Copy link
Collaborator

The changes here sound similar to a change we made in another issue so we may need to revisit the approach here. See #7126 (comment)

One key detail here is that we should avoid rendering the individual tiles themselves when we know it shouldn't be rendered based on outside factors like settings.

@eugene-manuilov
Copy link
Collaborator

IB ❌

  • It should take the widgetSlug as a prop and use the isKeyMetricActive selector above. If the metric is active, it should render the children prop and if it is not active, it should render the <WidgetNull> component which should also be passed as a prop to the <MetricTile> component.

@jimmymadon the problem with the proposed approach is that all widgets will have pulled their reports before we actually start to determine whether or not we need to render anything in the MetricTile component. This will generate a lot of unnecessary requests that we should avoid. The approach to use HOC suggested by @aaemnnosttv solve the problem of preventing unnecessary requests. We need to figure out how to avoid rendering the widget in HOC if its slug is not selected. Moving this ticket back to IB.

@eugene-manuilov
Copy link
Collaborator

What we should do is update HOC to accept a new argument slug that we will use in the wrapper component to verify whether the widget can be rendered or not.

@jimmymadon
Copy link
Collaborator Author

@eugene-manuilov Thanks for the suggestion here - @aaemnnosttv and I did discuss this option as well. In the end, we decided to go for a simpler solution as mentioned in the now updated IB. I have also updated the ACs here.

c.c. @aaemnnosttv @tofumatt

@jimmymadon jimmymadon removed their assignment Jun 13, 2023
@eugene-manuilov eugene-manuilov self-assigned this Jun 13, 2023
@eugene-manuilov
Copy link
Collaborator

Thanks, @jimmymadon. IB looks good now.

@jimmymadon
Copy link
Collaborator Author

@techanvil Assigning this back to you with a follow up PR based on the bug we discussed. I have also updated the QA brief.

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • With the userInput feature flag disabled, the Site Kit dashboard functioned as normal and there are no console errors.
  • With the userInput feature flag I tested the following:
    • The individual Key Metric widget tiles are not rendered within the Key Metrics widget area when the logged-in user has not answered the questionnaire nor picked their own metrics.
    • The correct tiles appear based on the answer to question 1 in the user input form.
    • The Key Metrics widget area is displayed and hidden by switching the toggle within Admin Settings.
    • The "Navigation chips" for Key Metrics are hidden, displayed and highlighted correctly depending on when Key Metrics are shown or hidden on the dashboard.
    • Checked that the navigation chips function as expected when the widget is hidden and displayed
Screenshots

image
image
image
image
image

@wpdarren wpdarren removed their assignment Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants