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

Ensure pills for widget contexts that are empty/inactive are not displayed #4289

Closed
felixarntz opened this issue Oct 27, 2021 · 4 comments
Closed
Labels
P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Oct 27, 2021

#4053 introduced a pill navigation for the unified dashboard that points to individual widget context "section" with anchor links. Per #4136, widget contexts can contextually be empty ("inactive") though, and in that case the respective pill should not be displayed.


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

Acceptance criteria

  • The pill navigation introduced in Scaffold Unified Dashboard tab bar with chip/pill dummy links #4053 should be updated as follows: For each of the pills, it should be checked whether the respective widget context is active, using the isWidgetContextActive selector from Hide any section of the dashboard that has zero widgets #4136. If it is not active, the pill should be skipped/not rendered.
    • For that purpose, each pill needs to be associated with the respective widget context (see default-contexts.js).
    • When on the main dashboard, the contexts should be the respective CONTEXT_MAIN_DASHBOARD_*.
    • When on the entity dashboard, the contexts should be the respective CONTEXT_ENTITY_DASHBOARD_*.

Implementation Brief

  • Using assets/js/component/DashboardNavigation,
    • use the useDashboardType hook to check whether we are on the main dashboard or entity dashboard.
    • import the CONTEXT_MAIN_DASHBOARD_* and CONTEXT_ENTITY_DASHBOARD_* constants from assets/js/googlesitekit/widgets/default-contexts.js
    • Based on the dashboard type, from the result of useDashboardType, query the core/widgets data store via the isWidgetContextActive selector, passing either the CONTEXT_MAIN_DASHBOARD_* or CONTEXT_ENTITY_DASHBOARD_* constant for every pill. Note that every pill is associated with the appropriate context. For e.g CONTEXT_MAIN_DASHBOARD_TRAFFIC and CONTEXT_ENTITY_DASHBOARD_TRAFFIC are associate to the Traffic tab.
    • Render the pill only if its appropriate context is active.

Test Coverage

  • No new tests to be added.

QA Brief

  • Enable unifiedDashboard feature flag.
  • Make sure Adsense Module is not connected.
  • In Main Dashboard, you should not see the Monetization Chip
  • Connect Adsense Module.
  • The Monetization Chip Should be visible now.

Changelog entry

  • Update dashboard navigation to hide navigation chips for empty areas.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature labels Oct 27, 2021
@felixarntz felixarntz self-assigned this Oct 27, 2021
@felixarntz
Copy link
Member Author

@tofumatt This issue actually brings the "contextual awareness" from #4136 to the pill navigation in #4053, which I believe wasn't captured in an existing issue yet.

@felixarntz felixarntz removed their assignment Oct 27, 2021
@asvinb asvinb assigned asvinb and unassigned asvinb Nov 1, 2021
@eugene-manuilov eugene-manuilov self-assigned this Nov 1, 2021
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov
Copy link
Collaborator

@kuasha420 could you please add QAB?

@kuasha420 kuasha420 removed their assignment Nov 23, 2021
@wpdarren wpdarren self-assigned this Nov 23, 2021
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • When Adsense Module is not connected, the Monetization Chip does not appear on the unified dashboard.

image

  • When Adsense Module is connected, the Monetization Chip is visible now.

image

  • I also repeated the same process for PageSpeed Insights.
  • Checked the behaviour was the same on small screen sizes.

@wpdarren wpdarren removed their assignment Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants