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

Create `ChipTabGroup` component

Open zutigrm opened this issue 1 year ago • 6 comments

Feature Description

New ChipTabGroup component should be implemented, to support the selection panel layout updates. Said component should handle group switching, and styles, but it will not contain the implementation of full metrics selection and lists. That will be handled in separate issue.

As part of this issue assets/js/components/SelectionPanel/SelectionPanelItems.js should be used as a starting point for creating a key metrics specific SelectionPanelItems which will be rendering the ChipTabGroup component instead of the current markdown. A new story for the said SelectionPanelItems should be added, to visually test and develop new component. It shouldn't replace the usage of generic SelectionPanelItems in this issue.

Basic group filtering should be implemented as part of this issue

Check New Metric Grouping Logic Within Sidebar/Selection Panel - Tabbed Functionality section and more particularly Implementation of tabbed containers and tab functionality sub-section of the design doc


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

Acceptance criteria

  • New ChipTabGroup component is created
  • It should consist of the new group chips, and tab panel which will list the KMW items
    • The chips list all groups implemented in 9376
    • Tab panel shows a list of key metric items from the currently active/selected group chip
  • Clicking on the inactive group chip should switch/replace the current metric list, by showing only the metric items from the selected group chip.
  • Only 1 group chip can be active at the time (shouldn't be possible to select more)
  • Current selection group chip will be listing the selected items, functionality will be expanded on in 9385 , so for now it can display an empty list.
  • It should match figma designs
  • New SelectionPanelItems component is also added to render the ChipTabGroup component
  • Refer to the New Metric Grouping Logic Within Sidebar/Selection Panel - Tabbed Functionality section and more particularly Implementation of tabbed containers and tab functionality sub-section of the design doc
  • As part of this issue only ChipTabGroup component within new key metrics specific selection panel items is added, together with the stories, excluding "new" badge implementation. It should support group chip switching only (filtering all KMW items by the active group), rest of the functionality, and implementation of new panel will come in later issue

Implementation Brief

  • Follow the figma design while implementing the following components

Add assets/js/components/KeyMetrics/ChipTabGroup/Chip.js

  • It should accept following props:
    • slug and label, which will be extracted from the group object - {SLUG: '...', LABEL: '...'}
    • isActive , a boolean value
    • selectedCount, number of selected metric items from the current group, if any.
    • onClick, a callback function. It should accept group slug as an argument, so it can switch the active group in the parent's component state
  • Add Button component with class googlesitekit-chip-tab-group__chip-item for example
  • Use the label prop for label.
  • Include selectedCount value as trailingIcon prop. You can wrap it in span and pass the value within parentheses ()
  • For onClick prop, pass the onClick callback prop, including the slug prop as an argument
  • If passed isActive prop matches the current slug, render an extra class in the button, for example googlesitekit-chip-tab-group__chip-item--active
  • Add assets/sass/components/key-metrics/_googlesitekit-chip-tab-group.scss to include needed styling for both Chip and TabGroup components

Add assets/js/components/KeyMetrics/ChipTabGroup/TabGroup.js

  • It should accept following prop:
    • metricItems - an array of metric item objects to render. This component is not concerned with filtering, and active group selection logic, it will only receive final list that should be output.
  • Wrap the markup with main div.googlesitekit-chip-tab-group__tab-item for example
  • Iterate over metricItems and render MetricItem component for each item, passing the needed props

Add assets/js/components/KeyMetrics/ChipTabGroup/index.js

  • It should accept following prop for now (rest of the logic will be implemented in 9385):
    • allMetricItems - will hold the array of metric objects retrieved from KEY_METRICS_WIDGETS list
  • Define a new group variable for Current selection, following the same structure how other existing groups are defined, for example {SLUG: 'currentSelection', LABEL: 'Current selection'}
  • Define a local state that will be used to update active group, and isActive value will be passed to the Chip component. By default Current selection slug should be used as initial value.
  • Define a callback which will be passed to the Chip component as onClick prop. It should accept a group slug as argument and update the local isActive state
  • Filter the allMetricItems prop by the group property, only the items which fulfil this criteria item.group === isActive should be returned, and assigned to a new variable, say activeMetricItems
    • The resulting activeMetricItems will be passed to the TabGroup component as the metricItems prop
  • Define selectedCount variable, for now assign it value 0 - this functionality will be implemented as part of the 9385
  • Wrap the markup in main div with class googlesitekit-chip-tab-group for example
  • Collect all ACR groups added in 9376, with Current selection being the first item in the array, and other groups should follow the order from the design.
    • Iterate over this list of groups, and render Chip component for each item, using the current group as group prop, and other props previously defined above

Add assets/js/components/KeyMetrics/MetricsSelectionPanel/SelectionPanelItems.js

  • Use assets/js/components/SelectionPanel/SelectionPanelItems.js as a starting point
  • It should be key metrics specific component, and render new ChipTabGroup component instead of the current markup.
    • Use KEY_METRICS_WIDGETS for the allMetricItems prop

Test Coverage

  • Add stories file, and include it in VRT for the new assets/js/components/SelectionPanel/SelectionPanelItems.js component, which should render the new selection panel layout

QA Brief

Changelog entry

zutigrm avatar Sep 19 '24 15:09 zutigrm

@zutigrm, could you please remove all technical details from AC and leave only requirements that the new component should match.

eugene-manuilov avatar Sep 26 '24 10:09 eugene-manuilov

@eugene-manuilov AC updated. Thanks

zutigrm avatar Sep 26 '24 13:09 zutigrm

Great! Thanks, @zutigrm. AC ✔️

eugene-manuilov avatar Sep 26 '24 13:09 eugene-manuilov

Thanks, @zutigrm. Mostly looks good, but let's rename active to be isActive and instead of using the group property, let's use slug and label props.

Also, the estimate seems to be very high, can you break down the estimate in a comment to have each component an estimate? Something like this:

  • assets/js/components/KeyMetrics/ChipTabGroup/Chip.js - X hrs
  • assets/js/components/KeyMetrics/ChipTabGroup/TabGroup.js - Y hrs
  • etc

eugene-manuilov avatar Oct 05 '24 10:10 eugene-manuilov

@eugene-manuilov

let's rename active to be isActive and instead of using the group property, let's use slug and label props.

Updated, thanks.

can you break down the estimate in a comment to have each component an estimate?

assets/js/components/KeyMetrics/ChipTabGroup/Chip.js - 2h assets/js/components/KeyMetrics/ChipTabGroup/TabGroup.js - 1h assets/js/components/KeyMetrics/ChipTabGroup/index.js - 3.5h assets/js/components/KeyMetrics/MetricsSelectionPanel/SelectionPanelItems.js - 1h And stories, probably around 1h

zutigrm avatar Oct 11 '24 08:10 zutigrm

Thanks, @zutigrm. IB looks good and 11 seems to be more reasonable for this component.

eugene-manuilov avatar Oct 11 '24 09:10 eugene-manuilov

QA Update ⚠

Tested this in storybook and here are my observations:

ITEM 1: Comparing with the chips list implemented in #9376 , some are not in the same order. Are we expecting to mirror the exact order? Also under 'Content performance', currently we have the item 'Top earning pages' but in #9376, it's supposed to be 'Top earning content'

Content performance:

Image

Image


ITEM 2: Should we be implementing the dots at the top of groups? Image attached for reference.

Image


ITEM 3: The AC references the following: New SelectionPanelItems component is also added to render the ChipTabGroup component What exactly are these and what should we be looking out for here?


ITEM 4: The current checkbox size is around 14 x 14. Based on Figma, it's 24 x 24.

Actual:

Image

Figma:

Image


ITEM 5: Based on figma, the checkboxes should be almost vertically centrally aligned in a section. Currently, it's not centrally aligned.

Actual:

Image

Figma:

Image


ITEM 6: On mobile, the groups are supposed to be in a single line with a sliding user interaction. Currently, the groups appear on 3 lines.

Current Selection > Content Performance appear on 3 lines: ![Image](https://github.com/user-attachments/assets/32bf57e2-2f86-4e74-b2f1-2e1ed4bcb13b)

It should be a single line: Image


ITEM 7: The select text at the bottom should be 14px instead of 12px.

Image

Image


ITEM 8: On mobile, the text 'Edit your personalized goals..etc.' should be 12px instead of 14px.

Image

Image


ITEM 9:

On mobile, the groups should be aligned to start on the same line as the top text. Image is attached for reference. The groups should start at the red line.

Image


ITEM 10: Sometimes when an item is selected, the text 'The metrics you selected require more data tracking. You will be directed to update your Analytics property after saving your selection.' will appear.

Nothing wrong with that but the position looks weird. It sits just below the different items and when you change groups, it moves around.

I think it's a better experience if it sits on top of the bottom section.

https://github.com/user-attachments/assets/03e87e39-754d-42c9-8a42-ec29a1c4274b


ITEM 11:

Font weight for 'Settings' is currently 600 but based on figma, it's 400.

Image

Image

kelvinballoo avatar Nov 08 '24 14:11 kelvinballoo

Hi @kelvinballoo thanks for your observations,

ITEM 2: Should we be implementing the dots at the top of groups? Image attached for reference.

This will be implemented in the separate ticket that is already planned for new badge and dots

ITEM 3: The AC references the following: New SelectionPanelItems component is also added to render the ChipTabGroup component What exactly are these and what should we be looking out for here?

This is code related, nothing visual yet

ITEM 1: Comparing with the chips list implemented in https://github.com/google/site-kit-wp/issues/9376 , some are not in the same order. Are we expecting to mirror the exact order? Also under 'Content performance', currently we have the item 'Top earning pages' but in https://github.com/google/site-kit-wp/issues/9376, it's supposed to be 'Top earning content'

We do not expect to be in the same order, they will be computed and listed in the code with same priority. It seems the KMW content is changed to 'Top earning content', we can update it to match.

Items 4-11, and 'Top earning content' this will be finalised in the current issue I am working on #9385 which extends this ticket and replaces the selection panel which will be available for proper testing directly from the dashboard (except for new badge and dots which will be done in #9386). CUrrent issue is more about ChipTab Component and laying down the core infrastructure

zutigrm avatar Nov 08 '24 14:11 zutigrm

QA Update ✅

Thanks @zutigrm , since all the required issues will be fixed under #9385, I will verify those there. I have already put a note in the QAB section for that.

As for this ticket, verifying the storybook, the following were concluded per the QAB:

  • The implementation matches the Figma design 90%. The minor issues that do not match have been documented in this ticket will be verified under #9385

  • Switching between groups shows different metrics belonging to that group

    https://github.com/user-attachments/assets/8082e7bb-e7e9-42f1-9102-eacdcb2df320
  • AC points have also been verified and are as expected.

Moving ticket to approval.

kelvinballoo avatar Nov 08 '24 18:11 kelvinballoo