Add PriceBenchmarkSuggestions component #2853

asvinb · 2025-04-14T16:50:11Z

Changes proposed in this Pull Request:

Closes #2828 .

Replace this with a good description of your changes & reasoning.

Screenshots:

Detailed test instructions:

Navigate to the Price Benchmark tab.
Verify that the "Price Benchmark & Suggestions" table displays data correctly.
In mobile view, ensure that only two columns are visible: "Product" and "Action".
Confirm that the "Change Price" button is non-functional and currently serves as a placeholder.
Ensure the UI implementation matches the provided designs in terms of layout, styling, and responsiveness.

Additional details:

Changelog entry

…stions-table

…estions-table

codecov · 2025-04-14T16:53:32Z

Codecov Report

Attention: Patch coverage is 31.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 67.3%. Comparing base (8aff2b7) to head (c47df83).
Report is 42 commits behind head on feature/2824-price-benchmarks.

Files with missing lines	Patch %	Lines
js/src/data/actions.js	0.0%	5 Missing ⚠️
js/src/data/reducer.js	0.0%	3 Missing ⚠️
js/src/data/resolvers.js	0.0%	2 Missing ⚠️
js/src/data/selectors.js	50.0%	1 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                         @@
##             feature/2824-price-benchmarks   #2853      +/-   ##
==================================================================
+ Coverage                             67.2%   67.3%    +0.2%     
==================================================================
  Files                                  817     334     -483     
  Lines                                24995    5157   -19838     
  Branches                              1252    1251       -1     
==================================================================
- Hits                                 16789    3473   -13316     
+ Misses                                8065    1545    -6520     
+ Partials                               141     139       -2

Flag	Coverage Δ
js-unit-tests	`67.3% <31.2%> (-<0.1%)`	⬇️
php-unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
js/src/data/action-types.js	`100.0% <ø> (ø)`
js/src/pages/price-benchmark/price/index.js	`100.0% <100.0%> (ø)`
js/src/data/selectors.js	`56.9% <50.0%> (-0.1%)`	⬇️
js/src/data/resolvers.js	`17.0% <0.0%> (-0.2%)`	⬇️
js/src/data/reducer.js	`80.0% <0.0%> (-1.2%)`	⬇️
js/src/data/actions.js	`14.7% <0.0%> (-0.3%)`	⬇️

... and 486 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

joemcgill

@asvinb I took a look through the WIP here and overall the setup seems good to me. I left a few question/comments but think you're on the right track

js/src/components/badge/index.js

joemcgill · 2025-04-15T01:36:11Z

js/src/data/actions.js

+			data,
+		};
+	} catch ( error ) {
+		handleApiError(


We'll probably need to add some handling here later for #2852.

js/src/pages/price-benchmark/effectiveness-indicator.js

joemcgill · 2025-04-15T02:11:20Z

js/src/pages/price-benchmark/price-benchmark-suggestions.js

+ * for desktop and mobile views.
+ *
+ * @return {JSX.Element} A div containing the PriceBenchmarkTable component.
+ */
 const PriceBenchmarkSuggestions = () => {


Will this component also render the MetricsNotice?

No, it's the PriceBenchmarkTable component which will render MetricsNotice.

In that case, I wonder if we can remove one level of nesting here and move all of the fields setup directly into the PriceBenchmarkTable component?

I originally had it like that @joemcgill :
c6020d5
But then i thought the PriceBenchmarkTable should only contain fields that should be part of the basic table. Then any components using PriceBenchmarkTable should pass their extra fields. I feel that can be more flexible if we have different table types in the future. Let me know what you think. Happy to go with your suggestion as well 😁

Is there a use case you have in mind where another component would need to use PriceBenchmarkTable? Seems like any custom tables that need different data could do so and wrap their own DataViews component.

So eventually, we’ll have the PriceBenchmarkAdjustments component. My main concern is that PriceBenchmarkTable will contain logic related to both suggestions and adjustments, even though we already have (or will have) dedicated components like PriceBenchmarkSuggestions and PriceBenchmarkAdjustments that encapsulate that logic. In other words, I’d prefer not to split some of logic across both PriceBenchmarkTable and PriceBenchmarkSuggestions. What do you think?

Ah, I see what you have in mind. Let's go with it for now and evaluate as the rest of the feature comes together. 👍🏻

joemcgill

@asvinb I've left a round of feedback on this PR. The most important things that need to be addressed are:

Ensuring we're using the expected shape from the REST API endpoint for price-benchmarks
Resolving the bundle watch issues.

For the latter, could we update the webpack config so the dataviews package isn't bundled into the vendors.js bundle? We will probably need to enqueue it specifically for this page once we do so the bundle isn't loaded on other pages where it is not being used.

Even then, the bundle size might still be too large, so we will likely need to figure out how to allow the max size for that package to be above the current threshold.

joemcgill · 2025-04-15T12:44:57Z

js/src/pages/price-benchmark/price-benchmark-suggestions.js

+ * for desktop and mobile views.
+ *
+ * @return {JSX.Element} A div containing the PriceBenchmarkTable component.
+ */
 const PriceBenchmarkSuggestions = () => {


In that case, I wonder if we can remove one level of nesting here and move all of the fields setup directly into the PriceBenchmarkTable component?

js/src/pages/price-benchmark/price-benchmark-table/index.js

js/src/pages/price-benchmark/price-benchmark-suggestions.js

js/src/pages/price-benchmark/price/index.js

tests/e2e/specs/price-benchmark/price-benchmark-suggestions.test.js

tests/e2e/utils/__fixtures__/price-benchmark-suggestions.json

joemcgill

Almost there, @asvinb. Unfortunately I found some issues with the change in data shape that need to be fixed before this is merged. Everything else looks non-blocking.

joemcgill · 2025-04-17T19:06:04Z

js/src/pages/price-benchmark/price-benchmark-table/index.js

+const BASE_FIELDS = [
+	{
+		id: 'image',
+		enableHiding: false,
+		enableSorting: false,
+		enableGlobalSearch: false,
+		label: __( 'Image', 'google-listings-and-ads' ),
+		render: ( { item } ) => {
+			return (
+				<img
+					src={ item.product.thumbnail }
+					alt={ item.product.title }
+					className="gla-price-benchmark-table__image"
+				/>
+			);
+		},
+	},
+	{
+		id: 'title',
+		enableHiding: false,
+		enableSorting: false,
+		enableGlobalSearch: true,
+		label: __( 'Product', 'google-listings-and-ads' ),
+		render: ( { item } ) => {
+			return item.product.title;
+		},
+	},
+	{
+		id: 'description',
+		enableHiding: false,
+		enableSorting: false,
+		enableGlobalSearch: true,
+		label: __( 'Description', 'google-listings-and-ads' ),
+		render: ( { item } ) => {
+			return <span>{ item.product.id }</span>;
+		},
+	},
+];


I noticed after the data shape updates that there is no validation on the item properties, which caused the app to crash with the following error:

TypeError: Cannot read properties of undefined (reading 'id')

We should probably use optional chaining or validate that the data prop passed to this component includes the expected properties to make this more robust.

Additionally, while testing this after applying the correct data shape, I noticed that the sorting functionality now also causes the app to crash:

TypeError: Cannot read properties of undefined (reading 'localeCompare')

The search functionality also seems to not be working, but am not sure that it was previously.

To reproduce the way I've tested this:

Open DevTools to the network panel in Chrome

Look for Fetch/XHR requests to /wp-json/wc/gla/mc/price-benchmarks (must have pretty permalinks enabled)

Right click the request and choose "Override Content"

Paste in the data from the tests/e2e/utils/__fixtures__/price-benchmark-suggestions.json file.

Refresh the page with DevTools still open

Same property validation applies to the TABLE_FIELDS in the PriceBenchmarkSuggestions component.

thanks @joemcgill . I've pushed a fix. Can you kindly check again?

joemcgill · 2025-04-17T19:16:11Z

js/src/pages/price-benchmark/price-benchmark-table/index.scss

@@ -0,0 +1,50 @@
+.gla-price-benchmark-table {


@asvinb I'm curious why so much of the dataviews style need to be overridden? Is this just to match the designs? For consistency with other WP UI, it would be nice if we could avoid doing so much customization.

Not a blocker, but something to discuss with @michaeleleder when this is ready to demo.

@joemcgill The main styling changes are:

Custom alignment for table cells. Some cells are left aligned and some are right aligned.

The image dimensions are 32x32 with a 2px border radius. By default, it's 60x60 with no border radius.

Removed the minimum height of 32px for the title to match the designs.

Sure we can discuss with @michaeleleder about that.

@asvinb I know the default DataViews may not have it, but right alignment for number columns is consistently used across multiple different sections that use DataViews in our development. The image dimension is also taken from a previous project (although I’m not sure about its dev status). I understand you don’t have access to those, but maybe @budzanowski could help?

Regarding the last bullet point, I’m not sure what it is referring to.

Thanks @michaeleleder The last bullet point refers to a min height of 32px applied to the title which adds some spacing between the title and product id. Here is the default:

Compared to the designs where it looks much more compact:

@joemcgill is correct.

It was pointed out to me that we should:

avoid "local overrides" for dataviews: this means do not have CSS overrides and use stock dataviews.

How much this complicate things?

@asvinb @budzanowski Seems like the spacing doesn’t fit the smaller images. It’s tricky because I’ve taken this from another project design (not sure about the dev status), and I think we’ll be using this in another project too, so we might need it in the future. But if now isn’t a good time to spend the effort, we can just use the large image.

These are the two different image sizes we have in our design systems. This one seems to be a combination of the small image and a two-line title:

@michaeleleder In that case, I'am happy to revert the CSS overrides for dataviews and only keep the alignments overrides. Let me know if that works for you.

@asvinb Okay with me, thanks!

joemcgill · 2025-04-17T19:17:16Z

js/src/pages/price-benchmark/price/index.js

+ * @return {JSX.Element} The rendered price component.
+ */
+const Price = ( { amount, highlight = false } ) => {
+	const { formatAmount } = useAdsCurrency();


Eventually, it would be nice to have some jest tests for this component that confirm this is working as intended with different currency types.

Added tests although the test are mostly to test the isNaN functionality and highlight.

joemcgill · 2025-04-17T19:21:27Z

package.json

+				"maxSize": "41 kB"
+			},
+			{
+				"path": "./js/build/wp-dataviews.js",
+				"maxSize": "242.57 kB"


As we discussed in Slack, including the @wordpress/dataviews package in plugins currently requires using a special bundle that includes all of the needed dependencies so this is cross compatible with different versions of the dependencies externalized to WP core.

Related GB discussion: WordPress/gutenberg#66825

For now, the larger bundle size is fine, but it we should try to improve this in a follow-up issue. For example, can we avoid the additional size in vendors.js?

Sure @joemcgill . Follow up ticket created: #2863

👋 I was asked if I could clarify this bit.

What Joe shared is correct: @wordpress/dataviews/wp (the one used here) comes with bundled dependencies, so it works properly in different environments (different WordPress versions, with/without Gutenberg plugin).

The alternative is using @wordpress/dataviews. In that case, the dependencies are externalized, which means DataViews will access them via the wp.* global. This is problematic if the plugin aims to be compatible with different environments: the global components may have a different API depending on the environment, which would cause issues.

Looking at the latest build-wp/index.js published as part of the @wordpress/dataviews npm package it seems to be about ~1.6Mb.

Thanks @oandregal ! I think in our case, it's better we use the @wordpress/dataviews/wp to ensure the app works properly in different environments since we are supporting previous WordPress versions. Can you kindly take a look at the suggested approach here, whether it makes sense to you please? We don't want to bloat the vendors chunk unnecessarily with dependencies from the @wordpress/dataviews package. Let me know what you think.

joemcgill

Looks good!

asvinb added 21 commits April 9, 2025 19:51

Add Badge component.

9179974

Add EffectivenessIndicator component.

7e25ec5

Add @wordpress/dataviews package.

52b57c2

Add basic styles and placeholder content.

ab6b5b2

Merge branch 'add/2826-price-benchmark-tab' into add/2828-price-sugge…

727701e

…stions-table

Merge branch 'feature/2824-price-benchmarks' into add/2828-price-sugg…

0136f0e

…estions-table

Add badge colors.

f2bfb16

Use new badge colors.

8a18e67

Tweak styles for table.

b74baf9

Use CurrentFactory.formatAmount function.

26a9ed0

Add Label component.

7f36e34

Add Price component.

550c1eb

Add PriceBenchmarkTable component.

c6020d5

Update suggestions table to use PriceBenchmarkTable component.

0ac927d

Add placeholder ChangePrice component.

6963cad

Update datastore.

566aa89

Merge branch 'feature/2824-price-benchmarks' into add/2828-price-sugg…

311e621

…estions-table

Add usePriceBenchmarkSuggestions hook.

95bb6db

Add correct way of getting effectiveness badge.

201592f

Remove test data.

79edc72

Add @wordpress/viewport package.

ff64912

asvinb requested a review from joemcgill April 14, 2025 16:50

github-actions bot added type: enhancement The issue is a request for an enhancement. changelog: add A new feature, function, or functionality was added. labels Apr 14, 2025

joemcgill reviewed Apr 15, 2025

View reviewed changes

asvinb added 3 commits April 15, 2025 12:52

Add unspecified type for effectiveness.

be438a1

Add E2E tests.

1b29a27

Add unspecified effectiveness test.

ce85471

asvinb marked this pull request as ready for review April 15, 2025 10:27

asvinb added 2 commits April 15, 2025 15:06

Fix font sizes

d7073ea

Add tests for mobile view.

a91842f

joemcgill linked an issue Apr 16, 2025 that may be closed by this pull request

PriceBenchmarkSuggestionsTable component #2828

Open

3 tasks

joemcgill requested changes Apr 17, 2025

View reviewed changes

asvinb added 8 commits April 17, 2025 11:14

Fix typo.

21b1fac

Add check for non numbers for Price component.

eb4f02a

Use same data format as the API.

333723a

Exclude @wordpress/dataviews package from vendors chunk.

6dd92ce

Tweak webpack config.

52b3bf8

Temporarily increase bundle size.

e30112f

Increase vendors bundle max size.

83db740

Add useCallback to callback function.

d865e18

joemcgill reviewed Apr 17, 2025

View reviewed changes

asvinb added 5 commits April 18, 2025 14:58

Add unit tests for Price component.

a185509

Change description to be string.

e9d0b35

Fix searching and logic to show fields.

daf93bc

Cast description to string.

86a80fd

Enable sorting by product title.

8904ea9

joemcgill approved these changes Apr 18, 2025

View reviewed changes

Remove dataview CSS overrides.

c47df83

asvinb mentioned this pull request Apr 22, 2025

PriceBenchmarkSuggestionsTable component #2828

Open

3 tasks

asvinb merged commit 7f3e4d2 into feature/2824-price-benchmarks Apr 23, 2025
7 checks passed

asvinb deleted the add/2828-price-suggestions-table branch April 23, 2025 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PriceBenchmarkSuggestions component #2853

Add PriceBenchmarkSuggestions component #2853

asvinb commented Apr 14, 2025 •

edited

Loading

codecov bot commented Apr 14, 2025 •

edited

Loading

joemcgill left a comment

joemcgill Apr 15, 2025

joemcgill Apr 15, 2025

asvinb Apr 15, 2025

joemcgill Apr 15, 2025

asvinb Apr 17, 2025

joemcgill Apr 17, 2025

asvinb Apr 17, 2025

joemcgill Apr 17, 2025

joemcgill left a comment

joemcgill Apr 15, 2025

joemcgill left a comment

joemcgill Apr 17, 2025

joemcgill Apr 17, 2025

asvinb Apr 18, 2025

joemcgill Apr 17, 2025

asvinb Apr 18, 2025

michaeleleder Apr 18, 2025

asvinb Apr 22, 2025

budzanowski Apr 22, 2025

michaeleleder Apr 22, 2025

asvinb Apr 22, 2025

michaeleleder Apr 22, 2025

joemcgill Apr 17, 2025

asvinb Apr 18, 2025

joemcgill Apr 17, 2025

asvinb Apr 22, 2025

oandregal Apr 22, 2025

asvinb Apr 22, 2025

joemcgill left a comment

Add PriceBenchmarkSuggestions component #2853

Add PriceBenchmarkSuggestions component #2853

Conversation

asvinb commented Apr 14, 2025 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Apr 14, 2025 • edited Loading

Codecov Report

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

asvinb commented Apr 14, 2025 •

edited

Loading

codecov bot commented Apr 14, 2025 •

edited

Loading