site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard
verified publisher icon google

Refactor the Key Metrics Selection Panel, extracting components for reuse in the Audience Segmentation Selection Panel.

Open techanvil opened this issue 1 year ago • 4 comments

Feature Description

In order to efficiently reuse the KM Selection Panel's markup, styling and presentation-related logic, we should extract a set of generic, presentational components from the Key Metrics Selection Panel, refactoring the panel to use these components in the process.

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

Acceptance criteria

  • The KM Selection Panel should be refactored such that its markup rendering is extracted to a set of generic presentational components.
  • Generic presentation-related logic should be extracted along with the markup where appropriate.
  • CSS classes used by these generic components should be renamed to remove references to Key Metrics.
  • The generic components and styling should be suitable for building additional Selection Panels, with the Audience Segmentation Selection Panel the first additional consumer.
  • The KM Selection Panel should be unchanged from the user's perspective.

Implementation Brief

Please refer the POC PR: https://github.com/google/site-kit-wp/pull/8676

Extracting components.

  • [ ] Create a directory SelectionPanel in assets/js/components/ and extract following components into it.
    • [ ] SelectionPanel.js this is the main component which will wrap the SideSheet component into it with the following props.
      • [ ] isOpen = Flag to tell whether panel is open or not.
      • [ ] onOpen - A function which gets executed once the panel is opened.
      • [ ] closeFn - A function which gets executed once the panel is closes.
      • [ ] children - This is useful to insert/display other components within the panel.
    • [ ] SelectionPanelHeader.js, it will display the panel heading and optionally the settings link. It will accept the following props.
      • [ ] onCloseClick - Callback when the panel closes.
      • [ ] title - Title/heading for the panel.
      • [ ] children - Component/content to display in the panel header.
    • [ ] SelectionPanelFooter.js please refer the component in POC PR. It will be responsible for displaying the error, footer content, cancel button and save button. This component will accept the following props.
      • [ ] savedItemSlugs - Array of slugs for the saved items in database.
      • [ ] selectedItemSlugs - Array of slugs for the selected items in the selection form inside CORE_FORMS store.
      • [ ] saveSettings - Callback to save selections.
      • [ ] saveError - Any error that may have occurred while saving the settings in saveSettings callback.
      • [ ] itemLimitError - Error to display when less/more than minimum/maximum allowable items are selected respectively.
      • [ ] minSelectedItemCount - Minimum items to be selected.
      • [ ] maxSelectedItemCount - Maximum allowable items to be selected.
      • [ ] isBusy - Flag to tell whether panel CTAs should be disabled because some other operation like saving the settings is in progress.
      • [ ] onSaveSuccess - Callback function to call when settings are saved successfully. This can be utilised to reset the dirty form status.
      • [ ] onCancel - Callback function to call when cancel button is clicked.
      • [ ] isOpen - Flag to tell if the selection panel is open.
      • [ ] closeFn - Callback function to call when closing the panel. It will be called on click of cancel button. It should reset any selections in selection form in CORE_FORMS store.
    • [ ] SelectionPanelItems.js - Component is used to render the currently selected items and other available items for selection.
      • [ ] It will also accept the ItemComponent prop, which is the component responsible for displaying the individual item. By default, this should be SelectionPanelItem component in case it is not passed.
    • [ ] SelectionPanelItem.js this component should expect the props sent by the SelectionPanelItems component which are id, slug, title, description, onChange, disabled, checked and children.
      • [ ] id - This will be used as key prop for item component as well as the id attribute for item in html.
      • [ ] slug - It will be used as checkbox value.
      • [ ] title - title for checkbox.
      • [ ] description - Description of the item, will appear below the title. This will be a react component and will be rendered as is.
      • [ ] onChange - Callback for checkbox selection. Will be called when item is selected/deselected.
      • [ ] checked - Flag to tell whether the item should be selected or not.
      • [ ] disabled - Whether the checkbox should be disabled.
      • [ ] children - Any additional children component that needs to be displayed after description.

Updating Key Metrics components.

  • [ ] Key Metrics components should utilize the extracted generic components so that we have a concrete key metrics component. The concrete components should pass props to the extracted components.
    • [ ] Update assets/js/components/KeyMetrics/MetricsSelectionPanel/index.js, use SelectionPanel component and pass the relevant props to it.
    • [ ] Add MetricsHeader, Metrics, CustomDimensionsNotice and MetricsFooter component as children to the SelectionPanel component.
    • [ ] Update MetricsHeader component. Pass the following props to SelectionPanelHeader component.
      • [ ] title - Heading text for the selection panel.
      • [ ] onCloseClick - Callback to trigger when selection panel is closed.
      • [ ] children - This should display the text under the main heading with the settings link. This is a component created with createInterpolateElement.
    • [ ] Update Metrics component so that it uses SelectionPanelItems component for selected items and other PanelItems component for available items.
      • [ ] Pass the title for current selection and available selections.
      • [ ] Pass the currently saved items and unsaved items as props. Logic for this should already be there in the Metrics component.
    • [ ] Update MetricsFooter component, it should use SelectionPanelFooter to compose the concrete component.
      • [ ] Please note that we need to evaluate the necessary props inside the component and pass it to generic component to make use of it.
    • [ ] Update MetricItem component to use SelectionPanelItem component. Pass other props as mentioned in the SelectionPanelItem component.

Updating CSS class names to more generic ones.

  • [ ] Rename all instances of googlesitekit-km-selection-panel-* and replace with googlesitekit-selection-panel-.
  • [ ] Also, remove any references to metrics or metric in classname. For example: googlesitekit-km-selection-panel-metrics__metric-item should be replaced by googlesitekit-selection-panel-item.
  • [ ] Create a directory selection-panel inside assets/sass/components/.
    • [ ] Move assets/sass/components/key-metrics/_googlesitekit-km-selection-panel.scss inside new directory.
    • [ ] Rename file to _googlesitekit-selection-panel.scss.
    • [ ] Change class names to the generic ones as we changed above.

Test Coverage

  • Update the e2e tests for the key metrics selection panel.
  • Update the stories for the key metrics selection panel.
  • Add the stories for generic selection panel component.
  • There should be no failing VRT ideally as we are not modifying anything visually, but fix any failing tests.

QA Brief

  • Open the Key Metrics selection panel and verify that it is absolutely unchanged.
  • In Storybook, check out the Components > Selection Panel story variants and verify that their behaviour and styling is aligned with the key metrics selection panel (there might be a few features not implemented but generally it should behave similarly).

Changelog entry

  • Enhance Key Metrics selection panel by introducing and reusing generic selection panel components.

techanvil avatar May 02 '24 15:05 techanvil

Hi @ankitrox, thanks for drafting the IB/POC. It's definitely heading in the right direction, however there are a number of aspects I would like to see addressed.

  • We should be aiming to reuse the CSS classes more than your POC demonstrates.
  • We should also look to share more common logic and structure, while keeping the refactor as simple as we can.
  • I think we can avoid the saved flag in the items and we don't need a suffix at this point either.

I figured it would be easiest to show you what I mean in more detail by putting together a POC myself: https://github.com/google/site-kit-wp/pull/8676/. I think this is most of the way there and the main thing remaining to do is to genericise the CSS class names.

Please take a look at my POC, consider my feedback, let me know your thoughts if you wish to discuss this, and/or update the IB accordingly.

techanvil avatar May 08 '24 12:05 techanvil

Thanks @ankitrox, nice work here so far on the IB.

I've made a few tweaks, to fix a couple of minor errors and remove some duplication, which you can see in the edit history.

Additionally - although I did mention you could take the approach of not listing all the props for each item and instead refer to the POC, where you have listed the props (which is totally fine), you've missed most of them for the SelectionPanelFooter component. Please can you provide the full list of props for this component too, for consistency?

techanvil avatar May 09 '24 10:05 techanvil

Thanks @techanvil for fixing typos and formatting.

I have expanded on SelectionPanelFooter component props as suggested. I haven't provided too much info on MetricsFooter because it would be repetitive.

Assigning you for review.

Thanks again!

ankitrox avatar May 09 '24 15:05 ankitrox

Thanks @ankitrox, that's great. IB LGTM! :white_check_mark:

techanvil avatar May 10 '24 09:05 techanvil

QA Status ⚠️

This is tested good overall:

  • CSS classes used by these generic components have been renamed to remove references to Key Metrics. ✅

    Screenshot 2024-05-22 at 17 25 02

  • The KM Selection Panel is unchanged from the user's perspective. Compared prod and develop and the panels are consistent with each other. ✅

  • In Storybook, the Components > Selection Panel story variants, their behaviour and styling is aligned with the key metrics selection panel ✅

  • Layout, margin, padding, font family , font sizes are consistent with Key Metrics panel ✅

  • Verified on Mobile, tablet and other browsers too and results are consistent across. ✅

  • The only behaviour that I noticed is different is when clicking a checkbox. Refer to the detailed video below ⚠️

    https://github.com/google/site-kit-wp/assets/125576926/c24303cc-ab2a-4388-92e8-81dc82b1168b

kelvinballoo avatar May 22 '24 13:05 kelvinballoo

Thank you for sharing your observation, @kelvinballoo!

  • The only behaviour that I noticed is different is when clicking a checkbox. Refer to the detailed video below ⚠️

Interesting observation. However, as long as the focus behaviour is unchanged in the actual application, we should be good to go here, which I just checked and ensured that it is.

See screen recording

https://github.com/google/site-kit-wp/assets/20284937/f676887b-2a22-4fef-89a4-9d60fd5e9292

The Storybook story for SelectionPanel uses a custom set of made-up items and relies on the local state to store the selection. The focus gets lost there most likely due to re-rendering events. I believe we can overlook that.

Please let me know if you have any other concerns, thank you, Kelvin!

nfmohit avatar May 22 '24 19:05 nfmohit

QA Updates ✅

Thanks for the explanation @nfmohit . That makes sense. Moving this ticket to Approval.

kelvinballoo avatar May 23 '24 13:05 kelvinballoo