Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.41k stars 1.99k forks source link

Investigate the consolidation of storage usage bars #93299

Open chad1008 opened 3 months ago

chad1008 commented 3 months ago

While working on https://github.com/Automattic/wp-calypso/pull/92989, we noticed that there are two different versions of the PlanStorageBar component:

Both components are built on top of the underlying ProgressBar component, and are similar in many ways. Each of them is rendered by the PlanStorage component which will, by default, use the first/general version of PlanStorageBar, unless an alternative is provided via the StorageBarComponent prop. That's what's currently being done to serve the specialized version on the Hosting Overview.

There are a few differences between the two - perhaps most notably that the general version of the component has a warning/alert feature to apply specialized css classes at designated percentages. This allows (for example) the progress bar to go from blue to yellow at 50%, and then red at 80%.

During our work on the Hosting Overview storage bar, we wound up copying some of that functionality into the specialized component, increasing the amount of duplicated code. With that in mind, we'd like to look at both components, where and how they are used, and explore unifying them into a single component with the props necessary to serve all of the currently existing use cases.

When looking into this, we'll need to search out not just usages of PlanStorageBar (paying close attention to which version is being imported), but also PlanStorage, which pops in in a few places, and will use PlanStorageBar by default.

With the recent changes to the Hosting Overview version of the component, I'm hopeful that the differences between these two components are small enough to make unification possible, especially if we make sure to maintain thing like the is-compact prop to avoid regressions along the way.

fredrikekelund commented 2 months ago

👋 @xavier-lc, I see you originally implemented client/hosting/overview/components/plan-storage-bar.tsx in https://github.com/Automattic/wp-calypso/pull/89752. Could you share your thoughts on the feasibility of merging that one and client/blocks/plan-storage/bar.jsx? It looks like you pursued that approach in https://github.com/Automattic/wp-calypso/pull/89786 but abandoned it.

xavier-lc commented 2 months ago

@fredrikekelund on https://github.com/Automattic/wp-calypso/pull/89786 I was attempting to extract the return null logic, because I think that the planHasFeature( sitePlanSlug, FEATURE_UNLIMITED_STORAGE ) bit didn't apply on the hosting overview. After that, I realized that I wouldn't be able to match the design with the HTML used on the original PlanStorageBar, so it made more sense to me to create a new one.

But they are not wildly different, so merging them should be feasible. I would loop a designer into the conversation, because the different looks might not make sense anymore.