Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create ChipTabGroup component #9378

Open
zutigrm opened this issue Sep 19, 2024 · 9 comments
Open

Create ChipTabGroup component #9378

zutigrm opened this issue Sep 19, 2024 · 9 comments
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 19, 2024

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

  • Check the storybook story ?path=/story/key-metrics-chiptabgroup--default
  • Ensure it matches the Figma design
  • Switching between groups should show different metrics belonging to that group
  • Current selection is not implemented in this issue, it will not show selection. Note that currently it will show graphic that is going to be shown and used in full screen variation that will be implemented in different issue.

Changelog entry

  • Add a new "Chip Tab Group" component.
@zutigrm zutigrm added Type: Feature New feature P0 High priority Team S Issues for Squad 1 labels Sep 19, 2024
@zutigrm zutigrm changed the title Create <ChipTabGroup /> component Create ChipTabGroup component Sep 19, 2024
@zutigrm zutigrm assigned zutigrm and unassigned zutigrm Sep 20, 2024
@eugene-manuilov eugene-manuilov self-assigned this Sep 26, 2024
@eugene-manuilov
Copy link
Collaborator

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

@zutigrm
Copy link
Collaborator Author

zutigrm commented Sep 26, 2024

@eugene-manuilov AC updated. Thanks

@zutigrm zutigrm assigned eugene-manuilov and unassigned zutigrm Sep 26, 2024
@eugene-manuilov
Copy link
Collaborator

Great! Thanks, @zutigrm. AC ✔️

@eugene-manuilov eugene-manuilov removed their assignment Sep 26, 2024
@zutigrm zutigrm self-assigned this Sep 26, 2024
@zutigrm zutigrm added Type: Enhancement Improvement of an existing feature and removed Type: Feature New feature labels Sep 26, 2024
@zutigrm zutigrm removed their assignment Sep 30, 2024
@eugene-manuilov eugene-manuilov self-assigned this Oct 5, 2024
@eugene-manuilov
Copy link
Collaborator

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

@zutigrm
Copy link
Collaborator Author

zutigrm commented Oct 11, 2024

@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 zutigrm assigned eugene-manuilov and unassigned zutigrm Oct 11, 2024
@eugene-manuilov
Copy link
Collaborator

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

@eugene-manuilov eugene-manuilov removed their assignment Oct 11, 2024
@zutigrm zutigrm self-assigned this Oct 22, 2024
@zutigrm zutigrm removed their assignment Nov 4, 2024
@tofumatt tofumatt assigned tofumatt and zutigrm and unassigned tofumatt Nov 4, 2024
@zutigrm zutigrm assigned tofumatt and unassigned zutigrm Nov 5, 2024
@tofumatt tofumatt removed their assignment Nov 6, 2024
@kelvinballoo kelvinballoo self-assigned this Nov 6, 2024
@kelvinballoo
Copy link
Collaborator

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.

9378metric.jumping.mov

ITEM 11:

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

Image

Image

@zutigrm
Copy link
Collaborator Author

zutigrm commented Nov 8, 2024

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 #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'

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 zutigrm removed their assignment Nov 8, 2024
@kelvinballoo
Copy link
Collaborator

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 Update key metrics selection to persist the current selection group #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 kelvinballoo removed their assignment Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Team S Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

4 participants