[Clov-406][BpkRating] Improve BpkRating component to allow its subtitle text truncation in edge cases #3872

Faye-Xiao · 2025-07-07T08:38:32Z

We have received feedback that in real production, BpkRating may not display all subtitle text content when the component is placed in a width-limited container.

Ideally, BpkRating only has 1~2 words in the subtitle. But it could be text overflow in a long language.
This PR is to improve this question. The solution is to make its subtitle truncated when the container width is not enough to display all the content. One condition is not to limit the width of its own component.

before	after	note
		default
		large size

Remember to include the following changes:

Faye-Xiao · 2025-07-07T08:40:05Z

packages/bpk-component-rating/src/BpkRating-test.tsx

+
+    expect(screen.getByText('Excellent')).toBeVisible()
+    expect(screen.getByText('672 reviews')).toBeVisible()
+    expect(screen.getByLabelText('4.6 Excellent 672 reviews')).toBeInTheDocument();


As we do not recommend using toMatchSnapshot, changed it to use jest expect element.

Faye-Xiao · 2025-07-07T08:40:27Z

packages/bpk-component-rating/src/BpkRating-test.tsx

+  const defaultProps = {
+    ratingScale: RATING_SCALES.zeroToFive,
+    size: RATING_SIZES.base,
+  };


Added default props for testing.

Faye-Xiao · 2025-07-07T08:41:20Z

packages/bpk-component-rating/src/BpkRating-test.tsx

-    expect(asFragment()).toMatchSnapshot();
+
+    expect(screen.getByLabelText('6.7 Average might recommend')).toHaveClass('bpk-rating');
+    expect(screen.getByText('/5')).toHaveClass('bpk-rating__scale');


As the ratingScale: RATING_SCALES.zeroToFive, it displays the max value.

Faye-Xiao · 2025-07-07T08:42:19Z

packages/bpk-component-rating/src/BpkRating-test.tsx

+    expect(screen.getByText('Low')).toBeVisible();
+    expect(screen.getByText('Bad option')).toBeVisible();
+    expect(screen.getByLabelText('-1.3 Low bad option')).toBeInTheDocument();
+    expect(screen.getByText('/5')).toHaveClass('bpk-rating__scale');


As the ratingScale: RATING_SCALES.zeroToFive, it displays the max value.

Faye-Xiao · 2025-07-07T08:42:30Z

packages/bpk-component-rating/src/BpkRating-test.tsx

+    expect(screen.getByText('Smashing')).toBeVisible();
+    expect(screen.getByText('Doubleplusgood')).toBeVisible();
+    expect(screen.getByLabelText('10 Super, smashing, great')).toBeInTheDocument();
+    expect(screen.getByText('/5')).toHaveClass('bpk-rating__scale');


As the ratingScale: RATING_SCALES.zeroToFive, it displays the max value 5.

skyscanner-backpack-bot · 2025-07-07T08:44:03Z

Visit https://backpack.github.io/storybook-prs/3872 to see this build running in a browser.

Faye-Xiao · 2025-07-07T08:46:03Z

packages/bpk-component-rating/src/BpkRating.module.scss

@@ -34,6 +34,7 @@

  &__text-wrapper {
    display: flex;
+    min-width: 0;


The container of text-wrapper uses the flex layout. The min-width:0 is to make its min width 0. Allow flex-item to shrink to 0, thus giving child elements a real chance to "squeeze".

There is a nested flex layout in the BpkRating

Faye-Xiao · 2025-07-07T08:54:23Z

packages/bpk-component-rating/src/BpkRating.module.scss

@@ -61,6 +62,12 @@
  }

  &__subtitle {
+    min-width: 0;
+    max-width: 100%;
+    flex: 1 1 auto;


The __subtitle is the second flex item in the flex container text-wrapper.

The flex: 1 1 auto; is to occupy the remaining space, can be reduced, with three sets of triggering ellipsis.

When flex-direction: row; in text-wrapper

min-width:0 -> to make its min-width 0, not use the default min-width: auto

When flex-direction: column; in text-wrapper

max-width: 100%; -> As flex direction is changed, they now take care of the height, not the width we want to truncate. So we can add a max-content to the container horizontally

See the related CSS spec https://www.w3.org/TR/css-flexbox-1/#min-size-auto

Faye-Xiao · 2025-07-07T08:54:54Z

packages/bpk-component-rating/src/BpkRating.module.scss

    color: tokens.$bpk-text-secondary-day;
+    text-overflow: ellipsis;
+    white-space: nowrap;
+    overflow: hidden;


3 sets of triggering text ellipsis.

Faye-Xiao · 2025-07-07T09:04:34Z

packages/bpk-component-rating/src/BpkRating.tsx

-  showScale: true,
-  title: null,
-};
-


Removed the propTypes and defaultProps and use TS type definition solution.

skyscanner-backpack-bot · 2025-07-07T09:35:25Z

Visit https://backpack.github.io/storybook-prs/3872 to see this build running in a browser.

skyscanner-backpack-bot · 2025-07-07T09:37:34Z

	Warnings
⚠️	Package source files (e.g. `packages/package-name/src/Component.js`) were updated, but snapshots weren't. Have you checked that the tests still pass?

Browser support

If this is a visual change, make sure you've tested it in multiple browsers.

Generated by 🚫 dangerJS against 19933a5

Faye-Xiao · 2025-07-07T09:49:43Z

The Percy failure https://percy.io/09e69e44/web/backpack/builds/41384044?utm_campaign=09e69e44&utm_content=backpack&utm_source=github_status_public came from the flaky test. A slight change that does not involve visuals.

GCpigsic · 2025-07-07T11:14:41Z

A stupid question:
Could it be that the screenshots for the large size—before and after—have been mixed up?

Faye-Xiao · 2025-07-07T11:32:47Z

Could it be that the screenshots for the large size—before and after—have been mixed up?

Yes. Good catch! I made a mistake. Corrected it.

Copilot

Pull Request Overview

This PR improves the BpkRating component by enabling subtitle text truncation in constrained containers and refactors the code and tests to modern patterns.

Adds CSS rules to truncate the subtitle with an ellipsis when there's insufficient width.
Refactors BpkRating to use inline default props, removes prop-types, and adds a catch-all type for extra props.
Updates tests to use @testing-library/react’s screen queries and removes legacy snapshots.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/bpk-component-rating/src/accessibility-test.tsx	Introduces `defaultProps` and updates accessibility test render
packages/bpk-component-rating/src/BpkRating.tsx	Removes `PropTypes`, adds inline defaults and rest index signature
packages/bpk-component-rating/src/BpkRating.module.scss	Adds `min-width: 0` and ellipsis styles for subtitle truncation
packages/bpk-component-rating/src/BpkRating-test.tsx	Replaces snapshots with `screen`-based assertions
packages/bpk-component-rating/src/snapshots/*	Deletes outdated snapshot file

Copilot · 2025-07-08T03:13:03Z

packages/bpk-component-rating/src/accessibility-test.tsx

  it('should not have programmatically-detectable accessibility issues', async () => {
    const { container } = render(
      <BpkRating
+      {...defaultProps}


The accessibility test render is missing the required value prop on <BpkRating>, which may lead to runtime errors or unexpected output. Add a value prop (e.g., value={3.6}) to the rendered component.

Copilot · 2025-07-08T03:13:03Z

packages/bpk-component-rating/src/BpkRating-test.tsx

        ariaLabel="4.9 Awesome It is a fantanstic place"
        title="Awesome"
        subtitle="It is a fantanstic place"
        value={4.9}
        showScale={false}
      />,
    );
-    expect(asFragment()).toMatchSnapshot();
+
+     expect(screen.queryByText('4.9')).not.toHaveClass('bpk-rating__scale');


This assertion checks the class on the numeric value element rather than verifying the absence of the scale element. To confirm the scale is not rendered, use something like expect(screen.queryByText('/5')).toBeNull().

Suggested change

expect(screen.queryByText('4.9')).not.toHaveClass('bpk-rating__scale');

expect(screen.queryByText('/5')).toBeNull();

This is a special case as it will display 4.9 because of the ratingScale: RATING_SCALES.zeroToFive. The bpk-rating__scale is for the case when showScale=true

Copilot · 2025-07-08T03:13:04Z

packages/bpk-component-rating/src/BpkRating.tsx

@@ -55,20 +54,20 @@ type Props = {
  subtitle?: string,
  title?: string | ReactNode,
  value: string | number,
+  [rest: string]: any


[nitpick] The index signature [rest: string]: any is very permissive and can hide invalid props. Consider extending React.HTMLAttributes<HTMLDivElement> (or the appropriate element interface) to provide stronger typing for spread props.

Suggested change

[rest: string]: any

I also tried to remove this type declaration, and the type check also passed. is there any reason that we add it?

Yes, it won't trigger TS check. The reason why I added is beacuse it is used in the below code.

backpack/packages/bpk-component-rating/src/BpkRating.tsx

Line 115 in a3d166c

<div className={classNames} aria-label={ariaLabel} role="figure" {...rest}>

And also referred to the similar code written.

backpack/packages/bpk-component-text/src/BpkText.tsx

Line 79 in a3d166c

[rest: string]: any;

Copilot · 2025-07-08T03:13:04Z

packages/bpk-component-rating/src/BpkRating.tsx

+          <span className={titleStyles}>
+            <BpkText
+              textStyle={titleTextSize}
+              tagName="span"
+              aria-hidden="true"
+            >
+              {title}
+            </BpkText>
+          </span>
        )}

-        {subtitle && (
+        {subtitle && (<span className={subtitleStyles}>
          <BpkText
            textStyle={subtitleTextSize}
            tagName="span"
            aria-hidden="true"
          >
-            <span className={subtitleStyles}>{subtitle}</span>
+            {subtitle}
          </BpkText>
+          </span>


[nitpick] The extra wrapper <span> around <BpkText> adds redundant markup. If BpkText supports a className or wrapperClassName prop, apply titleStyles directly to it to simplify the DOM structure.

We do not expect to make obvious changes to the DOM structure at this time. I don't know why it's designed as a nested SPAN here.

GCpigsic

Could we add storybooks for the truncated case?

Faye-Xiao · 2025-07-08T07:01:55Z

https://backpack.github.io/storybook-prs/3872

Hi @GCpigsic No strong need to add new case for that. Because we can see that in the current storybook use case if you click this link https://backpack.github.io/storybook-prs/3872/?path=/story/bpk-component-rating--visual-test-with-zoom&globals=viewport:mobile1.

skyscanner-backpack-bot · 2025-07-08T07:49:50Z

Visit https://backpack.github.io/storybook-prs/3872 to see this build running in a browser.

GCpigsic

LGTM

Faye-Xiao · 2025-07-08T10:12:32Z

Aproved the Percy build https://percy.io/09e69e44/web/backpack/builds/41408462/changed/2220746939?browser=chrome&browser_ids=64&group_snapshots_by=similar_diff&subcategories=unreviewed%2Cchanges_requested&utm_source=github_status_public&viewLayout=side-by-side&viewMode=new&width=1280&widths=375%2C1280 as there is no real visaul change

Faye-Xiao added 3 commits July 7, 2025 11:50

remove the React PropTypes defination in BpkRating

aabeaf4

modify BpkRating dom and css for the edge case

d00ba0e

improve unit test

ba3522a

Faye-Xiao commented Jul 7, 2025

View reviewed changes

Faye-Xiao added the patch Patch production bug label Jul 7, 2025

fix unit test error and add type for rest

7a4250b

GCpigsic requested a review from Copilot July 8, 2025 03:11

Copilot AI reviewed Jul 8, 2025

View reviewed changes

GCpigsic reviewed Jul 8, 2025

View reviewed changes

Merge branch 'main' into CLOV-406

19933a5

GCpigsic approved these changes Jul 8, 2025

View reviewed changes

Faye-Xiao merged commit 44eb6aa into main Jul 8, 2025
10 checks passed

Faye-Xiao deleted the CLOV-406 branch July 8, 2025 10:13

Faye-Xiao added V38 Planned this change in V38 version and removed V38 Planned this change in V38 version labels Jul 9, 2025

Faye-Xiao mentioned this pull request Jul 16, 2025

[NO-JIRA][BpkRating] Fixed the issue where some BpkRating attributes were required #3885

Merged

12 tasks

	expect(screen.queryByText('4.9')).not.toHaveClass('bpk-rating__scale');
	expect(screen.queryByText('/5')).toBeNull();

[Clov-406][BpkRating] Improve BpkRating component to allow its subtitle text truncation in edge cases #3872

[Clov-406][BpkRating] Improve BpkRating component to allow its subtitle text truncation in edge cases #3872

Uh oh!

Conversation

Faye-Xiao commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Faye-Xiao Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skyscanner-backpack-bot bot commented Jul 7, 2025

Uh oh!

Faye-Xiao Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Faye-Xiao Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skyscanner-backpack-bot bot commented Jul 7, 2025

Uh oh!

skyscanner-backpack-bot bot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Browser support

Uh oh!

Faye-Xiao commented Jul 7, 2025

Uh oh!

GCpigsic commented Jul 7, 2025

Uh oh!

Faye-Xiao commented Jul 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

GCpigsic Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GCpigsic left a comment

Choose a reason for hiding this comment

Uh oh!

Faye-Xiao commented Jul 8, 2025

Uh oh!

skyscanner-backpack-bot bot commented Jul 8, 2025

Uh oh!

GCpigsic left a comment

Faye-Xiao commented Jul 7, 2025 •

edited

Loading

Faye-Xiao Jul 7, 2025 •

edited

Loading

Faye-Xiao Jul 7, 2025 •

edited

Loading

Faye-Xiao Jul 7, 2025 •

edited

Loading

skyscanner-backpack-bot bot commented Jul 7, 2025 •

edited

Loading

GCpigsic Jul 8, 2025 •

edited

Loading