Site-kit-wp: Modify `whenActive` HOC to distinguish between active and connected state

[felixarntz]

The whenActive higher-order component behaves in a slightly inaccurate way at the moment, as it only checks whether the given module is not connected. It should also check whether the module is active and allow for a different fallback based on the outcome.

---

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

Acceptance criteria

• The whenActive HOC should be improved as follows:
  
  
  
  • fallbackComponent should be renamed (capitalized) to FallbackComponent to indicate that it requires a WPComponent.
  
  
  • In addition to FallbackComponent, it should be able to receive an IncompleteComponent.
  
  
  • It should also check whether the given module is active.
  
  
  • If the module is not active, render the FallbackComponent if given, or return null otherwise.
  
  
  • If the module is active but not connected, render the IncompleteComponent if given, or the FallbackComponent if given, or return null otherwise.
  
  
  • Otherwise (i.e. if the module is active _and_ connected), render the underlying component.
  
  
• The DashboardPageSpeedWidget should be updated to no longer check the module activation/connected state in the component, but rather rely on the whenActive HOC. It should pass ActivateModuleCTA with only the module slug as prop for fallbackComponent and CompleteModuleActivationCTA with only the module slug as incompleteComponent. The DashboardPageSpeedCTA component should be removed.
• All usages of whenActive that currently pass an AnalyticsInactiveCTA as FallbackComponent should be expanded to pass a CompleteModuleActivationCTA with only the module slug as IncompleteComponent.

Implementation Brief

• Modify whenActive HOC within /assets/js/util/when-active.js

  • Capitalize fallbackComponent param to be FallbackComponent to indicate that we're passing a component vs an element.
  • Modify JSDoc definition for FallbackComponent param to be an WPComponent|undefined to better indicate the type passed in.
  • Add IncompleteComponent as a param, including JSDoc definition (matches FallbackComponent).
  • Modify this line to grab the module object using the getModule selector rather than using the isModuleConnected selector.
  • Modify the conditional on Line 57 to immediately return null if module is a falsy value as getModule will return an object if a module is returned.
  • Follow the acceptance criteria's conditional logic to return FallbackComponent, IncompleteComponent or null depending on module state.

• Modify DashboardPageSpeedWidget component within /assets/js/modules/pagespeed-insights/components/dashboard/DashboardPageSpeedWidget.js

  • All component logic except for the Widget component return can be removed as this will be handled within the whenActive HOC. The DashboardPageSpeedCTA is no longer needed, the import can be removed.

  • Wrap the export for DashboardPageSpeedWidget with the whenActive HOC using the following object properties:

  • whenActive( {
    moduleName: 'pagespeed-insights',
    FallbackComponent: () => <ActivateModuleCTA slug="pagespeed-insights" />,
    IncompleteComponent: () => <CompleteModuleActivationCTA slug="pagespeed-insights" />,
    } )(DashboardPageSpeedWidget)
    

• Modify DashboardBounceRateWidget, DashboardGoalsWidget, & DashboardUniqueVisitorsWidget within /assets/js/modules/analytics/components/dashboard/

  • Modify whenActive argument object to include IncompleteComponent: () => <CompleteModuleActivationCTA slug="analytics" />

Test Coverage

Visual Regression Changes

QA Brief

• Disconnect and reset your site data.
• Reconnect your test site to the sitekit.google.com test site.
• Check your newly set up dashboard and you will see many of the widgets not active (showing messages like "PageSpeed Insights module needs to be configured".).
• To test the active but not connected state, click "SET UP ANALYTICS", auth with Google but don't complete the configuration steps once you return to SiteKit.
• Instead go back to the dashboard and you will see the analytics widgets now showing the active but not configured state ( showing messages like "%s module setup needs to be completed".)

Changelog entry

• Enhance whenActive higher-order component to accept a FallbackComponent as well as an IncompleteComponent prop.

[felixarntz]

Note that, while this is blocked by #2299, we can already start implementation - it's only the last small bit of changes here that is blocked by the other issue.

[felixarntz]

@caseymorrisus The IB looks good, just one thing:

Modify JSDoc definition for fallbackComponent param to be an Object as we will now be passing this (and incompleteComponent) as React elements rather than components (<Component slug="example" /> vs Component)

I think we should keep the existing approach of passing a component, as that is also indicated by the name of this argument, and that's our practice elsewhere. In other words, we should pass it like fallbackComponent: () => <Component slug="example" />, not fallbackComponent: <Component slug="example" />.

There are some other points in the IB where you're referring to passing a React element instead of a component, so let's adjust those bits accordingly.

[caseymorrisus]

@felixarntz Apologies ahead of time for the wall of text, there's probably a much more eloquent way to express this opinion.

I think there's some area for improvement with the implementation here as this is forcing us to use createElement which shouldn't be necessary in an environment where JSX is enabled (see React docs). Doing a search of the codebase (from what I can see) we're only passing components this way within Widget.js and when-active.js which can both be refactored without much effort based on the following info.

From my experience, many React libraries (Material UI comes to mind initially) tend to follow a similar set of rules when passing a component as a prop (or function arg, I'll use prop from here on out for brevity). If the prop name is lowercase, you must pass an instantiated component (element/node), whereas if the prop name is capitalized, you must pass an uninstantiated component (function/class). This makes things easier when using these props within child components as the alternative requires extra steps to get things to work without throwing.

<Component
  lowercase={ <ExampleComponent foo="prop from parent" /> }
  Capitalized={ ExampleComponent }
/>

There's a few benefits to this. This quickly lets you know at a glance which type of data is acceptable and how the prop should be used. This also signals if the prop is controlled strictly from the parent or if the child component is going to modify the component (useful for debugging). Using a lowercase prop and passing an uninstantiated component is why we need to use createElement within when-active.js and why we need to rename the header and footer props to Header and Footer within Widget.js as they would throw an error otherwise (inferred as HTML elements rather than components due to capitalization). Modifying how we apply these props fixes both of these issues.

The above component's definition could look something like:

function Component( { lowercase, Capitalized } ) {
  return (
    <div>
        {lowercase}
        <Capitalized foo="prop from child" />
    </div>
  )
}

Both Widget.js and when-active.js can use instantiated components (lowercase) as the child component doesn't pass in any props. The component prop passed in is rendered as-is without modification. I've found that passing uninstantiated (capitalized) components are much less common, but still valid.

An even less common pattern (but I have needed before) is a component that has props modified by both the parent and child components. This can be achieved using the uninstantiated (capitalized) version alongside an anonymous function (similar to what we require now). See the following:

<Component
  Capitalized={ ( { bar } ) => <ExampleComponent foo="prop from parent" bar={ bar } }
/>

Definition:

function Component( { Capitalized } ) {
  return (
    <div>
        <Capitalized bar="prop from child" />
    </div>
  )
}

Result:

<ExampleComponent foo="prop from parent" bar="prop from child" />

As another example where this makes things a little more difficult on ourselves is when you're passing a string (or other primitives) as a prop. With a lowercase prop, I'd expect <Component footer="text" /> to work but this would throw an error in our case. We'd need to write <Component footer={() => "text"} /> whereas <Component FooterComponent={() => "text"} /> is more obvious to me that a component is required (I personally like to use capitalization and component in the prop name to be double sure).

Let me know how you feel about the above or if you have any questions.

[aaemnnosttv]

Thanks for the detailed write-up @caseymorrisus !

I think there's some area for improvement with the implementation here as this is forcing us to use createElement which shouldn't be necessary in an environment where JSX is enabled

I'm not sure that it's _forcing_ us to use createElement – if wrappedComponent was cased as WrappedComponent I believe we could then simply return <WrappedComponent {...props} />, no? 🤔 This is of course related to your suggestions, but I guess my point is that nothing is forcing us to use the wrappedComponent casing, that's all 😄

With that said, I'm all for going with this convention so 👍👍 from me – I actually did the same thing in my recent PR for changes to the AdSense/Analytics report table.

Regarding the terms used here ((un)instantiated component), I think it's a bit more useful to refer to these in terms of the types that we use (particularly from @wordpress/element), and/or prop-types since we don't use React directly.

• WPElement / ReactElement / PropTypes.element / <MyComponent /> – "instantiated component"
• WPComponent / ComponentType / PropTypes.elementType / MyComponent – "uninstantiated component"

So assuming @felixarntz is onboard with adopting this convention, then we should also open at another issue (possibly multiple) to update other areas where we pass elements or component types as props/arguments.

---

Regarding the current IB

Modify JSDoc definition for fallbackComponent param to be an Object as we will now be passing this (and incompleteComponent) as React elements rather than components (<Component slug="example" /> vs Component)

This would be a WPElement rather than an Object

Otherwise, LGTM 👍

---

General feedback:

• Please avoid referencing specific line numbers because these can easily be inaccurate as soon as another PR is merged. Better to reference the code with a GitHub _permalink_ which also then becomes a rich embed where the code you're referencing is visible in the IB so we don't need to try to reverse-engineer the specific line you were referring to 😄
• It's okay to omit details which are enforced by linting or otherwise required, like adding a parameter doc. Good to call out changes though 👍

[felixarntz]

@caseymorrisus Thanks for the detailed explanation. The approach of using capitalized prop names for WPComponent and uncapitalized prop names for WPElement makes a lot of sense, so let's adopt that. I think it would be worth applying this change to core/modules's registerModule and core/widgets's registerWidget actions too, where some arguments require a WPComponent while using a lower-case name. That can be addressed in a separate issue though.

Regarding this specific issue: While we don't currently pass any props to the component, I'd rather adopt the more flexible approach where we require a WPComponent and not a WPElement. This will allow us to pass props in the future, which we could document, and then developers of these components could consider adopting them - or not. Even if passing an instantiated component (WPElement) appears to be more common, I'm not sure what the benefits are of that - IMO the extra flexibility of passing props to a WPComponent is worth choosing that one.

Regardless of whether approach we choose, we should be able to eliminate the use of createElement and use JSX instead.

In other words, I think in the current IB we should update the two arguments to accept a WPComponent (or undefined, since they shouldn't be required), and both of them should get capitalized names.

[felixarntz]

IB ✅

[benbowler]

I've completed this work, I'm just waiting on the blocking ticket to provide the AnalyticsInactiveCTA and CompleteModuleActivationCTA components.

[benbowler]

Completed, tested and added a QA brief.

[wpdarren]

QA Update: Fail ❌

@benbowler I'm going to be fussy here (sorry) but it's not a fail as such. The message Analytics module setup needs to be completed does not have a full stop. The other messages do, so going for consistency.

Verified:

After not completing Analytics set up, on the dashboard, for the analytics widgets, it is now showing the active but not configured state. Before and After screenshots.

[benbowler]

Nice one thanks @wpdarren. On it.

[benbowler]

The full stop change based on QA feedback from Darren is ready for review.

[aaemnnosttv]

@wpdarren thanks for raising the inconsistency with the sentences; good eye! After checking with @felixarntz, he confirmed that single sentences should not have a full-stop at the end in most cases. I've opened an issue to address this plugin-wide.

With that said, this observation need not block this issue as it does not deal with changes to those components.

Moving this back to QA for a final pass, if needed.

[wpdarren]

QA Update: Pass ✅

Verified:

After not completing Analytics set up, on the dashboard, for the analytics widgets, it is now showing the active but not configured state. Before and After screenshots.