site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Create new `googlesitekit-components` package for basic Material 2 components (in preparation for Material 3)

Open felixarntz opened this issue 3 years ago • 2 comments

A new JS entry point googlesitekit-components should be implemented, in which all basic (Material 2) components are bundled. The file name for it should be specific to the Material version though, i.e. googlesitekit-components-gm2.js. This is in preparation for #5802 (see related https://github.com/google/site-kit-wp/issues/5802#issuecomment-1251368851).


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

Acceptance criteria

  • All Site Kit components from assets/js/components for which there is an equivalent component in @material/web should be moved into a new assets/js/googlesitekit/components-gm2 directory.
  • A new assets/js/googlesitekit/components-gm2/index.js file should be added that imports all these React components and adds them as a property on a Components object (similar to e.g. in assets/js/googlesitekit/data/index.js).
  • A new assets/js/googlesitekit-components-gm2.js file should be added that just imports the above object and exposes it on a googlesitekit.components JS global (similar to e.g. in assets/js/googlesitekit-data.js).
  • The above entry point file should be added as a new asset in PHP, using the name googlesitekit-components. It should become a dependency to any asset that depends on any of these UI components (likely any JS files that have any components, i.e. most of them; maybe not the pure data store ones).
  • All imports (across the entire plugin) for any of those components which are now in the new googlesitekit-components asset should going forward be imported from 'googlesitekit-components' rather than relatively.
  • The new entry point / asset should also be added as an external, so that these imports in a production bundle are resolved as coming from the googlesitekit.components global, but otherwise from the assets/js/googlesitekit-components-gm2.js file.

Implementation Brief

Other than the regular IB requirements, the IB should also include the exact list of components to migrate to the new "package", based on their "counterpart" in the @material/web package.

  • Create a new assets/js/googlesitekit/components-gm2 directory and move the following components from assets/js/components to this new directory:
    • Button
    • Checkbox
    • Chip
    • Dialog
    • Menu
    • ProgressBar
    • Radio
    • Switch
    • Any other components that are equivalent in @material/web.
  • Create a new assets/js/googlesitekit/components-gm2/index.js file that imports all the components from the above directory and adds them as a property on a Components object (similar to e.g. in assets/js/googlesitekit/data/index.js).
  • Create a new assets/js/googlesitekit-components-gm2.js file that imports the above object and exposes it on a googlesitekit.components JavaScript global variable (similar to, e.g., in assets/js/googlesitekit-data.js).
  • In Assets::get_assets() method using the Script constructor, add the new googlesitekit-components asset and the source googlesitekit-components-gm2.js.
  • Add googlesitekit-components as a dependency to the following assets and any asset that depends on any of these UI components:
    • googlesitekit-user-input
    • googlesitekit-dashboard-splash
    • googlesitekit-dashboard-details
    • googlesitekit-dashboard
    • googlesitekit-settings
    • Any other assets that depend on any of the above components
  • Add googlesitekit-components as a dependency in all the Modules' setup_assets methods:
    • AdSense:setup_assets()
    • Analytics::setup_assets()
    • Analytics_4::setup_assets()
    • Idea_Hub::setup_assets()
    • Optimize::setup_assets()
    • PageSpeed_Insights::setup_assets()
    • Search_Console::setup_assets()
    • Tag_Manager::setup_assets()
    • Thank_With_Google::setup_assets()
  • Add googlesitekit-components as an external in the webpack.config.js file.
  • Add googlesitekit-components as an entry point and set the source to assets/js/googlesitekit-components-gm2.js in the webpack.config.js file.
  • Add googlesitekit-components to settings.import/core-modules in the .eslintrc.json file.
  • Update all the imports for the above components to import from googlesitekit-components rather than relatively.

Test Coverage

  • No new tests are to be added.
  • Ensure all the existing tests are passing.

QA Brief

  • This doesn't include any visual changes in the UI. It just changes how UI components are referenced in the background.
  • Check out the entire Site Kit workflow and verify that there are no regressions.
  • Try running googlesitekit.components in the browser console in Site Kit screens and verify that it returns an object with components (e.g. Button, Checkbox, Chip, etc.).

Changelog entry

felixarntz avatar Oct 03 '22 16:10 felixarntz

@aaemnnosttv @eugene-manuilov @techanvil Added this issue based on https://github.com/google/site-kit-wp/issues/5802#issuecomment-1251368851 and related conversations last week.

@eclarke1 @FlicHollis This should be started ASAP to unblock the following GM3 Phase 1 work.

felixarntz avatar Oct 03 '22 17:10 felixarntz

IB ✅

tofumatt avatar Oct 12 '22 20:10 tofumatt

Check out the entire Site Kit workflow and verify that there are no regressions.

@nfmohit Reading through the QAB and I feel we need some more detail here to test.

When you suggest workflow, do you mean setting up Site Kit, setting up modules, making changes to settings. Or are we doing a full smoke test of everything here? Looking at the IB it seems quite in-depth and touches on user input, different components, so I want to make sure that everything is captured in my testing.

wpdarren avatar Oct 26 '22 09:10 wpdarren

Thank you for your question, @wpdarren.

Apologies for not clarifying. I did mean a full smoke test since the components exist everywhere and we want to make sure we're covering every spot.

Let me know if you have any further questions, thank you!

nfmohit avatar Oct 26 '22 09:10 nfmohit

@nfmohit re.

Try running googlesitekit.components in the browser console in Site Kit screens and verify that it returns an object with components (e.g. Button, Checkbox, Chip, etc.).

Do you know the full code I should run in console?

I have checked all of the components and completed a smoke test and have not found any issues.

wpdarren avatar Oct 31 '22 09:10 wpdarren

@wpdarren All you need to run is googlesitekit.components. Here's an example of the code and output:

image

Let me know if you have any further questions. Thank you!

nfmohit avatar Oct 31 '22 14:10 nfmohit

QA Update: ✅

@nfmohit ah as simple as that! 🤦 Thanks for the clarification.

Verified:

  • I checked out the entire Site Kit workflow including ensuring that epics behind the feature flag continue to work. No regressions were found during my testing.
  • When running googlesitekit.components in the browser console in Site Kit screens and it returns an object with components (e.g. Button, Checkbox, Chip, etc.).

image

wpdarren avatar Nov 01 '22 03:11 wpdarren

Approval ⚠️

The implementation here looks good, however taking a closer look at @material/web, there are a few more components in there which we already somewhat have equivalents for in Site Kit, which should preferably also be in this new components-gm2 package. These are:

  • Select (which we import directly from material-components) --> in GM3 it looks like this is covered by Menu (in addition to regular menus, basically it doesn't seem to make a difference between a select and a more complex menu)
  • TextField (which we import directly from material-components) --> we should create our own wrapper for it that comes with GM2 Input built-in, and then refactor the usages to rely on our new component, as this is in line with GM3 where it is a single component
  • potentially even Link --> this is part of Button in GM3 (see md-text-button)

These are not blockers for this issue, but I think we may want to open a follow up issue to think about how to deal with them.

@nfmohit @tofumatt @techanvil What are your thoughts on the above?

felixarntz avatar Nov 03 '22 16:11 felixarntz

⚠️ Approval

Similar to #4863, this entry point shouldn't be bundled as part of Module Entry Points as it's intended to be independent from the rest of our internal modules. i.e. as an external, it shouldn't need the common runtime.

This package isn't used yet so this detail need not block the release.

aaemnnosttv avatar Nov 03 '22 21:11 aaemnnosttv

@tofumatt @techanvil @aaemnnosttv I have opened #6112 and #6113 as follow up issues to address the two above (non release blocking) comments.

felixarntz avatar Nov 05 '22 00:11 felixarntz