Admin Settings: Tracking section creates layout shift on load

aaemnnosttv commented 3 years ago

Bug Description

The admin settings section for Tracking contains a single component for controlling the user's opt-in to anonymous usage tracking. In most cases the component shows as expected, but under certain conditions can result in a layout shift due to an unhandled loading state.

https://user-images.githubusercontent.com/1621608/117275969-435d8900-ae67-11eb-836d-f52e7a330a67.mp4

Steps to reproduce

Clear session storage to wipe any existing request cache
Navigate to the settings page and wait 1 or more seconds before switching to the Admin Settings tab
Notice the contents of the Tracking section are blank for some period of time while loading

Additional Context

This is due to preloading of REST routes, for which core/user/data/tracking is included, however the preloaded data is only valid for a very short period of time, after which a request will be made as usual. That combined with the OptIn component's behavior to render nothing when loading create this layout shift/flicker on load.

https://github.com/google/site-kit-wp/blob/a194a8f034d72bf2776746399182b3c7f1068662/assets/js/components/OptIn.js#L61-L63

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

Acceptance criteria

The Checkbox component should receive support for a new boolean loading prop, in which case it should render a small loading spinner that takes up the exact same space as the checkbox would itself (i.e. no content shifts!).
- The loading spinner should only replace the little box itself, not any labels next to it.
The OptIn component should pass the new loading prop to its Checkbox when the data whether tracking opt-in is enabled or not is not loaded (yet).
The Checkbox used by the admin-bar-toggle in the Plugin Settings section should also be updated to use this new loading state while its value is being resolved

Implementation Brief

Inside assets/sass/vendor/_mdc-checkbox.scss

Add a new style rule so that if .spinner is a child of .mdc-checkbox, it has margin: 0. This means that the spinner will sit squarely in the space occupied by the checkbox.

Inside assets/js/components/Checkbox.js

import the Spinner component from ./Spinner.js
add a new loading prop, which takes a boolean and defaults to false
- make sure to add this to the propTypes definition
Refactor the component so that if loading is true, the Spinner is returned with its isSaving prop set to true.

Importantly, make sure that the Spinner is returned in a div.mdc-form-field > div.mdc-checkbox like the normal Checkbox output. This will ensure that it has identical height and no content shift will occur. For example:

  <div className="mdc-form-field">
      <div className="mdc-checkbox">
        { loading ? 
          <Spinner isSaving /> 
          : /* Existing Checkbox output... */ 
         }
      </div>

      { /* Note that the label is returned in the outer div */} 
      <label htmlFor={ id }>{ children }</label>
  </div>

Inside assets/js/components/OptIn.js:

Remove the logic where null is returned if enabled === undefined
Instead, pass a loading prop to the Checkbox and set its value to be enabled === undefined so that when enabled is still loading (i.e. is undefined), the value of the prop will be true.

Inside assets/js/components/settings/SettingsPlugin.js:

Pass a loading prop to the Checkbox and set its value to be true if showAdminBar has not yet loaded, e.g. loading = { typeof showAdminBar !== "boolean" }

Test Coverage

No new tests needed.

Visual Regression Changes

Images that deal with the admin settings might need updating if they reflect the loading state of the opt in.

QA Brief

The issue described above should be fixed
Both of the checkboxes on the admin settings page (/wp-admin/admin.php?page=googlesitekit-settings#/admin-settings) - 'Plugin settings' and 'Tracking' - should show a spinner as they load, though this might be too fast to observe in practice.

Changelog entry

Enhance some checkboxes with a spinner when its value is being loaded.

aaemnnosttv commented 3 years ago

Anything to add/change here @felixarntz ?

felixarntz commented 3 years ago

@aaemnnosttv Good catch! Should we maybe use a progress bar like we have in setup flows instead of PreviewBlock? We only use PreviewBlock for widgets, not in setup/settings areas, so that would be a bit inconsistent.

aaemnnosttv commented 3 years ago

@felixarntz I thought about the progress bar too, but that seems a bit busy for what will eventually be a single checkbox with some text. I think using a PreviewBlock would be more appropriate because it fills a specific amount of space whereas I think the progress bar may still shift the layout without additional wrapping/styles.

Another thing I thought about would be to just render everything but set the checkbox as disabled until loaded at which point the checked state would be set as well. That would be the least disruptive, but I also wouldn't want it to look like the opt-in is becoming checked programmatically (as if it wasn't before and was becoming checked not by the user's action). It would be nice if we could render a spinner in the place of the checkbox.

felixarntz commented 3 years ago

@aaemnnosttv I think the ProgressBar has a prop for its height, but even if not, we could still render the progress bar in a way that it is vertically center-aligned in a fixed-height div, or modify the component itself. I don't think we need to use PreviewBlock just because it already supports props to avoid layout shifts. The PreviewBlock usage currently happens only in widgets, elsewhere throughout Site Kit we tend to use ProgressBar, so I'd prefer to do that here as well. We could potentially even use the small variant of the ProgressBar which we do e.g. for the Analytics setup dropdowns.

aaemnnosttv commented 3 years ago

@felixarntz thinking about this a little more, I think both preview block and progress bar are overkill for this case and are not ideal for preventing a layout shift since the content filling the layout here is entirely text with the exception of the checkbox. It's the checked state of the checkbox which is the only thing this is waiting on, so it feels like overcompensating a bit by waiting on that to show anything.

What if we just replaced the checkbox with a loading spinner while loading? That way it it would consistently result in no shift on load (as long as size of spinner and checkbox were matched but that's very doable) and the layout as a whole would be more stable overall – replacing the whole block at once is still a big change visually even without a shift. Inline progress bars make more sense for using in areas where the user's primary focus is (e.g. in a module setup form).

What do you think?

felixarntz commented 3 years ago

@aaemnnosttv Hmm, how trivial would it be to replace the checkbox itself with a loading spinner but keep the checkbox label the same? I'd think this requires us to adjust the Checkbox component itself?

I'd propose an even simpler suggestion to not have a loader at all at instead simply disable the checkbox? This definitely ensures the same formatting. Normally I'd say a loading spinner is more accurate, but here this is such a minor piece that makes me think it may not be worth more effort.

aaemnnosttv commented 3 years ago

I'd propose an even simpler suggestion to not have a loader at all at instead simply disable the checkbox?

@felixarntz that was one of the ideas I mentioned a few months ago as well 😄 https://github.com/google/site-kit-wp/issues/3304#issuecomment-833675610 The only concern I have with that is that in the user's experience the checkbox will go from being unchecked and disabled to non-disabled and checked (if they had opted-in) which could look like we're programmatically checking the box for them since the change in disabled state is very subtle. That's why I think the spinner makes the most sense, in that it's a familiar pattern and we wouldn't show the actual input until its checked state was known.

Alternatively, we could go the initially disabled route with no spinner, but reduce the opacity of the entire component until it was loaded so that it was more clear that the whole thing is disabled until loaded, that way it would be much more clear. If we only style the checkbox as disabled, it will be very easy to miss.

felixarntz commented 3 years ago

@aaemnnosttv That's a fair point. Let's go with adding supporting for a loading state to Checkbox.

eugene-manuilov commented 2 years ago

Thanks, @johnPhillips. Let's define CSS styles in the appropriate .scss files instead of passing it via props.

aaemnnosttv commented 2 years ago

@eugene-manuilov @johnPhillips the loading state we're introducing here is also relevant to the checkbox for the admin bar toggle in the SettingsPlugin component. This doesn't result in a layout shift like the tracking one does, but its value is loaded in the same way (see #4038).

We don't have to include this fix in this issue, but I think it's small enough (and rather related) that we could fix this detail as part of this issue as well. It would be a bit odd if only the tracking checkbox had such a loading state but the other did not. We could fix this in #4038, but that would introduce a dependency that we don't need otherwise.

Thoughts @felixarntz?

felixarntz commented 2 years ago

@aaemnnosttv @eugene-manuilov @johnPhillips Fixing this for all occurrences makes sense to me, +1 for doing that here.

aaemnnosttv commented 2 years ago

SGTM @felixarntz 👍 I've updated the ACs with one additional point here which should be super simple to implement. I think it could be as simple as loading={ typeof showAdminBar !== "boolean" }.

johnPhillips commented 2 years ago

Thanks all, I think I've caught everything raised above. Over to you @eugene-manuilov for another pass 👍

eugene-manuilov commented 2 years ago

IB :heavy_check_mark:

wpdarren commented 2 years ago

QA Update: ⚠️

@aaemnnosttv I am seeing that the two checkboxes for Plugin Settings and Tracking are now slightly misaligned, unless it is my eyes? We fixed this on #4038 so wondering if something has been overwritten, could you take a look?

Edit: The more I look at it, the more I think it’s aligned :sweat_smile: I think what’s confusing my brain is the font sizes are different between the two checkboxes, assuming because one has more text?

wpdarren commented 2 years ago

QA Update: ✅

With a fresh pair of eyes, I can see that the checkboxes are aligned.

Verified:

Can confirm that the layout shift does not occur.
Both of the checkboxes on the admin settings page show a spinner as they load.
Additional QA: disabled the checkboxes, cleared session storage and refreshed the page. No issues.
Additional QA: enabled the checkboxes, cleared session storage and the spinners reappeared on refresh.

I cleared the session storage and waited around 15 seconds. The spinner appears very quickly but caught it on the screencast here.

https://user-images.githubusercontent.com/73545194/135624460-58373833-feb1-4956-95ee-c70139f692c8.mp4

google / site-kit-wp