Site-kit-wp: Require SVGs for all images and remove unnecessary image support

[caseymorrisus]

Feature Description

All existing images should be removed and replaced with SVGs as we will only support SVGs going forward. Existing logic for handling images can be removed to simplify logic.

We currently need to explicitly append a global variable (global._googlesitekitLegacyData.admin.assetsRoot) to every image source that we use. This is a bit tedious and could be handled better via a Webpack loader that appends this variable to every image source automatically.

See this conversation for the origins of this issue.

---

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

Acceptance criteria

• All plugin usage of non-vector images (e.g. .png, .jpg, ...) should be replaced with using SVG.
  
  
  
  • Note most non-vector images will be removed via #2285 or #2290 anyway.
  
  
  • Please ping @felixarntz / @marrrmarrr with a list of images where we need SVG replacements that we don't have.
  
  
• The assets/images folder should be removed and it should be removed from the build process.
• The smallImage and winImage props of the Notification component should be removed to encourage usage of vector images.
• Storybook stories using images should be updated accordingly and checked that they display as expected as well.

Implementation Brief

• Please ping @felixarntz / @marrrmarrr with a list of images where we need SVG replacements that we don't have.
• Remove allImagesLoaded logic from ./storybook/preview-head.html which can be found here.

• Replace existing images within /assets/images/, delete the directory, and remove from build process.

  • Within package.json remove preimagemin, imagemin, prestorybook, & postbuild:storybook scripts.
  • Remove imagemin-cli, imagemin-jpegtran, imagemin-optipng, & imagemin-webpack as dependencies.
  • logo-g_white_small.png
  • Imported within /assets/js/googlesitekit-base.js to load images for the parent menu item.
  • Added to admin menu within /includes/Core/Admin/Screen.php.
  • Use the source of the latest google dashicon
  • add_menu_page accepts a base64-encoded SVG data URI as the $icon_url param, existing implementation uses an image which is a bit more straightforward to implement.
  • Rather than storing a hardcoded string, we can generate this on-the-fly using something like the following:
    php add_menu_page( $this->args['title'], __( 'Site Kit', 'google-site-kit' ), $this->args['capability'], $this->slug, '', 'data:image/svg+xml;base64,' . base64_encode(file_get_contents($context->path( 'path/to/file.svg' ))) );
  • rocket.png
  • Used within /assets/js/components/legacy-notifications/dashboard-setup-alerts.js as rocketImage which is passed as the winImage prop for the Notification component.
  • Used within /stories/notifications.stories.js as rocketImage which is passed as the winImage prop for the Notification component.

  • Remove winImage prop and replace with an SVG by importing the image directly and passing to the WinImageSVG prop for the Notification component:

    import RocketImageSVG from '../path/to/file.svg';
    
    <Notification
      // ... other props
    WinImageSVG={ RocketImageSVG }
    >
    

  • sun-small.png
  • Passed into alert within /includes/Modules/AdSense.php.
  • thumbs-up.png
  • Used within /assets/js/components/settings/SettingsModules.js as thumbsUpImage which is passed as the smallImage prop for the Notification component.
  • Used within /stories/notifications.stories.js as thumbsUpImage which is passed as the smallImage prop for the Notification component.

  • Remove smallImage prop and replace with an SVG by importing the image directly and passing to the SmallImageSVG prop for the Notification component:

    import ThumbsUpImageSVG from '../path/to/file.svg';
    
    <Notification
      // ... other props
    SmallImageSVG={ ThumbsUpImageSVG }
    >
    

• Remove existing image logic within the Notification component at /assets/js/components/legacy-notifications/notification.js

  • winImage and smallImage props are no longer used, remove conditional and rendering logic.

    Test Coverage

  • Existing tests should still pass, no need to add new tests.

Visual Regression Changes

• Should remain the same or if new SVG images look different (new design), it should be okay to accept changes.

QA Brief

Changelog entry

[aaemnnosttv]

@felixarntz while the suggested change here would simplify our usage of the few images we use, perhaps we should redefine this issue to replace all images with svgs since images seem to be mostly a legacy thing for us (do we plan on adding images in the future?). Most of these are very much icon-like as it is (not sure if we can find the vector versions for all of these – if not perhaps we could replace them?). Several of them are specific to the legacy set up, legacy splash app, and publisher wins (which are being removed in #2285) as well. Of course we could keep support for images too, but they definitely seem to be more of an exception to the rule.

[felixarntz]

@aaemnnosttv I like the idea of changing our overall approach to only rely on SVGs going forward. IMO we don't need to fix/improve image handling then, and we could even consider removing image-specific props and behavior, to set a clear precedent that all images going forward should be SVGs. If we ever need to introduce an image that doesn't make sense as SVG (like a photo maybe), we could revisit, but I don't see that need coming up anytime soon.

I'll make this dependent on #2285 and #2290 since those will already remove a bunch of legacy images, and then we can take care of replacing the remaining ones with SVGs.

[aaemnnosttv]

@felixarntz ACs LGTM. We could also remove our use of imagesLoaded from our .storybook/preview-head.html which I added somewhat recently when attempting to fix the instability we were seeing although it ultimately didn't solve the issue at the time. Perhaps we should document this somewhere for the future, should we ever need to restore support for images.

[felixarntz]

@caseymorrisus @eclarke1 Looks like the remaining images that we'll need to get SVG versions/replacements of are:

• logo-g_white_small.png
• rocket.png
• sun-small.png
• thumbs-up.png

Can we define an Asana task to find or recreate the above four images as SVGs?

[aaemnnosttv]

@caseymorrisus

[ logo-g_white_small.png ] Added to admin menu within /includes/Core/Admin/Screen.php.

This one could use a bit more clarification on what is needed here since this image the only one referenced in PHP as part of the admin menu item for Site Kit. In particular, we need to update the way the image is provided to add_menu_page via the $icon_url parameter. As noted in the linked docs, this supports a "base64-encoded SVG using a data URI, which will be colored to match the color scheme" which is what we want. "This should begin with 'data:image/svg+xml;base64,'." As an example, this is how it is implemented in the AMP plugin:
https://github.com/ampproject/amp-wp/blob/d048e956b1fde897feb3523e0475f2c71e2101f6/src/Admin/OptionsMenu.php#L141-L148
https://github.com/ampproject/amp-wp/blob/d048e956b1fde897feb3523e0475f2c71e2101f6/src/Admin/OptionsMenu.php#L34-L39

If we can, I think it would be preferable to avoid storing a hardcoded base64 encoded string and do that part on-the-fly.

Note, this may require some duplication since the source svg file won't exist in a production build since these are bundled into the JS instead.

Regarding this icon specifically, I think we can use the source of the latest google dashicon. While $icon_url also supports using a dashicon, the google icon used to be for Google Plus and only very recently became the standard Google icon so we wouldn't be able to safely use that and be consistent across all versions of WP that Site Kit supports.

---

Also, would you please complete the last two sections of the IB for tests+VRT? These should always be filled out even if there are no changes to be made – then that would be fine to put there, but let's make sure nothing is missed in the process 👍

[aaemnnosttv]

Thanks @caseymorrisus ! Almost there, just a few details left to work out here.

Rather than storing a hardcoded string, we can generate this on-the-fly using something like the following:

add_menu_page(
  $this->args['title'],
  __( 'Site Kit', 'google-site-kit' ),
  $this->args['capability'],
  $this->slug,
  '',
  'data:image/svg+xml;base64,' . base64_encode(file_get_contents($context->url( 'path/to/file.svg' )))
);

This wouldn't work as-is since svg files are no longer included in production builds. Even if they were, this would result in an HTTP request for the URL of the svg on every page load which we shouldn't do.

Perhaps you meant $context->path( 'path/to/file.svg' ) instead? This would be much more preferable but would require that we preserve or otherwise make the source file available since it will currently only be part of the JS bundles.

Regarding the replacements, you mention instances where the image is currently used, e.g. props to Notification. In cases like this, would you please elaborate a little to mention how they should be replaced?

In particular, for sun-small.png which you noted as "Passed into alert within /includes/Modules/AdSense.php" – let's define how cases like this would be handled where the icon to use is coming dynamically from the server side.

[caseymorrisus]

@aaemnnosttv Concerning SVGs being added to production builds, my current idea is to copy SVGs (ideally only those that are used) to the dist directory (copied later for release). This could be handled fairly easily using the CopyWebpackPlugin. This would need to be added as a dependency.

@felixarntz GET:notifications within AdSense.php currently passes an image src to the Notification component winImage prop (sun-small.png) whereas the new SVG prop requires a component to be passed in. Should we create a one-off prop to allow us to pass a string here from PHP or is it possible to remove the requirement for this entirely?

[aaemnnosttv]

@caseymorrisus regarding the use of CopyWebpackPlugin, this would work but would bypass any configuration we have for svg optimization in the process, would it not? Ideally we could use our existing dependencies to "build" the one svg into the dist directory for use by PHP. If there isn't a simple solution for this using the build, then it might be a good idea to use a similar approach to the AMP plugin and just keep the svg string in PHP since it will not be changing any time soon (and not fuss with the build at all).

One idea about the notification component – maybe modules could have the option of registering their own Notification component and if present, notifications for that component would use its component, otherwise use the generic one. That way this graphic could live in that component and the server wouldn't need to handle it any differently. Thoughts @felixarntz ?

[aaemnnosttv]

@felixarntz we also have includes/Core/Notifications/Notification.php which has an image key in its args. I think these are intended to be remote images though for the most part?

[felixarntz]

@aaemnnosttv @caseymorrisus It looks like the image argument of Notification is only used by the AdSense notification. The remote notifications (Notifications) do not use the image argument, so let's remove this argument and its usage entirely.

For now, to keep the original UI, let's take care of that part fully in JS: For any notification returned by modules/{moduleSlug}/data/notifications, the sun SVG should be included. AdSense is the only module that currently has any notifications anyway. We'll likely revisit this in the near future anyway, but for now that should be a good working approach.

I wouldn't wanna add more complexity add this point around notifications. We'll holistically revisit notifications throughout Site Kit once we'll tackle the Notification Center epic.