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

Basic comparison grid component #88045

Merged
merged 13 commits into from
Mar 4, 2024

Conversation

markbiek
Copy link
Contributor

@markbiek markbiek commented Feb 29, 2024

The only existing comparison grid components we have currently are heavily tied to the plans page. This adds a basic component that's more generic. (Currently only tested with 2 columns)

Related to #87905

Proposed Changes

For the new Site Migration flow, we have a step which will show the benefits of migrating over importing and we need some basic UI for it. I envisioned a comparison grid like the Plans pages. Unfortunately, all of the existing components are heavily tied to the Plans page.

This adds a new component to display a more generic comparison grid.

The goal here is to get some feedback about the component itself and any tweaks we might need to make. For the first iteration, we're only going to worry about having 2 columns. We'll mobile styles next (in a separate PR) and then larger numbers of columns.
image

Testing Instructions

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

The only existing comparison grid components we have currently are heavily tied to the plans page. This adds a basic component that's more generic. (Currently only tested with 2 columns)
@markbiek markbiek added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components New Component Adds a new common UI component labels Feb 29, 2024
@markbiek markbiek requested a review from a team February 29, 2024 19:19
@markbiek markbiek self-assigned this Feb 29, 2024
Copy link

github-actions bot commented Feb 29, 2024

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@gabrielcaires
Copy link
Contributor

@markbiek I liked to see the component on the storybook, it helped me a lot to see the component usage. 👍 .
I left some comments to improve the overall component architecture.

@andres-blanco andres-blanco self-assigned this Mar 1, 2024
@markbiek
Copy link
Contributor Author

markbiek commented Mar 1, 2024

@andres-blanco You may already be thinking about this but the HTML markup changes you suggested will probably make it way easier to support lots of columns and work on mobile.

It might be worth adding new storybook stories for different numbers of columns.

@gabrielcaires Is there a way to simulate a mobile view in a story?

@andres-blanco
Copy link
Contributor

@andres-blanco You may already be thinking about this but the HTML markup changes you suggested will probably make it way easier to support lots of columns and work on mobile.

It might be worth adding new storybook stories for different numbers of columns.

@gabrielcaires Is there a way to simulate a mobile view in a story?

I've pushed changes that should support multiple columns and basic stacking on mobile

Copy link
Contributor

@gabrielcaires gabrielcaires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left more suggestions but there is one case that requires more investigation:

Having fluid grid + gap led the layout to have an extra space after the last element.

image


const GridVariations = () => (
<>
<StandaloneComparisonGrid>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need the prefix Standalone and it making hard to read
Maybe we can use just:
ComparisonGrid
Column << imported from the same package module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that there is an existing ComparisonGrid component that's specific to the plans page. I wanted something to make it clear that this was a generic, general purpose component.

But I am super open to naming suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the StandAlone prefix but renamed the columns to Column. I felt as mark regarding avoiding confusion with the older ComparisonGrid

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this open either way if you feel like we should still drop / replace the prefix

@matticbot
Copy link
Contributor

matticbot commented Mar 1, 2024

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug 87905-update/site-migration-import-v-migrate on your sandbox.

@andres-blanco
Copy link
Contributor

I left more suggestions but there is one case that requires more investigation:

Having fluid grid + gap led the layout to have an extra space after the last element.

image

I moved to a flex display instead of grid and now it seems to behave correctly:
image

@markbiek markbiek removed their assignment Mar 4, 2024
@markbiek
Copy link
Contributor Author

markbiek commented Mar 4, 2024

LGTM!

I can't officially approve it since I opened the PR. But everything looks great.

@sixhours sixhours self-requested a review March 4, 2024 16:56
Copy link
Contributor

@sixhours sixhours left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a starting point! 👍

@andres-blanco andres-blanco merged commit e6ac8b9 into trunk Mar 4, 2024
13 checks passed
@andres-blanco andres-blanco deleted the 87905-update/site-migration-import-v-migrate branch March 4, 2024 16:57
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 4, 2024
@tyxla
Copy link
Member

tyxla commented Feb 7, 2025

Folks, are we still planning to use this component? It's been a year, and it's still unused.

In case we mean to use it, should it really be in @automattic/components or could it be moved inline where we mean to use it?

In case we don't want to use it, I'm happy to clean it up.

@tyxla
Copy link
Member

tyxla commented Feb 7, 2025

Here's a PR to clean it up: #99490

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components New Component Adds a new common UI component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants