Skip to content

refactor(pill-cloud): migrate PillCloud from Flow to TypeScript #4723

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.

Sign up for GitHub

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

Open
bonchevskyi wants to merge 1 commit into
base: master
Choose a base branch
from
Open

refactor(pill-cloud): migrate PillCloud from Flow to TypeScript #4723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/components/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,9 @@ export * from './menu';
export { default as PlainButton } from './plain-button';
export * from './plain-button';

export { default as PillCloud } from './pill-cloud';
export * from './pill-cloud';

export { default as Portal } from './portal';
export * from './portal';

Expand Down
0 src/components/pill-cloud/PillCloud.js → src/components/pill-cloud/PillCloud.js.flow
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions ...omponents/pill-cloud/PillCloud.stories.js → ...mponents/pill-cloud/PillCloud.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React, { useState } from 'react';

import PillCloud from './PillCloud';
import PillCloud, { PillCloudOption } from './PillCloud';
import notes from './PillCloud.stories.md';

const pills = [
const pills: PillCloudOption[] = [
{ value: 0, displayText: 'Box' },
{ value: 1, displayText: 'Fox' },
{ value: 2, displayText: 'Socks' },
Expand All @@ -23,7 +23,7 @@ const pills = [
];
export const regular = () => {
// eslint-disable-next-line react-hooks/rules-of-hooks
const [selectedOption, setSelectedOption] = useState(pills[5]);
const [selectedOption, setSelectedOption] = useState<PillCloudOption>(pills[5]);
return (
<div>
<PillCloud
Expand Down
45 changes: 45 additions & 0 deletions src/components/pill-cloud/PillCloud.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import * as React from 'react';
import classNames from 'classnames';
import isEqual from 'lodash/isEqual';

import Button from '../button';
import { ButtonProps } from '../button/Button';

export interface PillCloudOption {
/** Text shown on the pill */
displayText: string;
id?: string;
/** Value used as the pill key and resin target */
value: string | number | null;
Comment on lines +8 to +13

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Files matching PillCloud:"
fd -i 'PillCloud' . || true

echo
echo "Relevant source:"
if [ -f src/components/pill-cloud/PillCloud.tsx ]; then
  wc -l src/components/pill-cloud/PillCloud.tsx
  cat -n src/components/pill-cloud/PillCloud.tsx
fi

echo
echo "Search PillCloudOption usage:"
rg -n "PillCloudOption|usePill|data-resin-target|\\.value" src || true

Repository: box/box-ui-elements

Length of output: 50375

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "PillCloud tests:"
if [ -f src/components/pill-cloud/__tests__/PillCloud.test.tsx ]; then
  wc -l src/components/pill-cloud/__tests__/PillCloud.test.tsx
  cat -n src/components/pill-cloud/__tests__/PillCloud.test.tsx
fi

echo
echo "Story options:"
if [ -f src/components/pill-cloud/PillCloud.stories.tsx ]; then
  cat -n src/components/pill-cloud/PillCloud.stories.tsx
fi

echo
echo "React key/null behavior probe:"
node - <<'JS'
const reactKey = String(null);
const dataResinTarget = null;
console.log(JSON.stringify({ reactKey, dataResinTarget, type: typeof dataResinTarget }));
JS

Repository: box/box-ui-elements

Length of output: 5776

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Button component:"
for f in src/components/button/Button.tsx src/components/button/Button.js.flow; do
  if [ -f "$f" ]; then
    echo "--- $f ---"
    wc -l "$f"
    cat -n "$f"
  fi
done

Repository: box/box-ui-elements

Length of output: 8614

Use a stable, unique option key for PillCloud.

option.value is nullable and not required to be unique, but it is used as both the React key and data-resin-target at lines 31 and 36. Duplicate values can destabilize React reconciliation, and id is optional; prefer a guaranteed unique non-null key or validate this contract, including regression tests for duplicate/null values.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/pill-cloud/PillCloud.tsx` around lines 8 - 13, Update
PillCloudOption and the PillCloud rendering logic so each option uses a
guaranteed unique, non-null identifier for both the React key and
data-resin-target instead of nullable, potentially duplicated value. Prefer
making the existing id required and enforcing uniqueness, or validate and reject
invalid duplicate/null identifiers; add regression tests covering duplicate and
null values.

}

export interface PillCloudProps {
/** Props forwarded to each pill Button */
buttonProps?: Partial<ButtonProps> & Record<string, unknown>;
/** Called with the option when a pill is clicked */
onSelect?: (option: PillCloudOption) => void;
/** Options to render as pills */
options: Array<PillCloudOption>;
/** Currently selected options */
selectedOptions?: Array<PillCloudOption>;
}

const PillCloud = ({ options, onSelect, selectedOptions = [], buttonProps = {} }: PillCloudProps) => (
<div className="bdl-PillCloud pill-cloud-container">
{options?.map(option => (
<Button
key={option.value}
className={classNames('bdl-Pill', 'bdl-PillCloud-button', 'pill', 'pill-cloud-button', {
'is-selected': selectedOptions.find(op => isEqual(op, option)),
})}
onClick={onSelect ? () => onSelect(option) : undefined}
data-resin-target={option.value}
{...buttonProps}
Comment on lines +27 to +37

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Protect internally controlled PillCloud props in both implementations.

Both implementations spread buttonProps after generated props, allowing callers to remove required pill classes or bypass onSelect. Merge custom class names and callbacks while preserving the component’s internal behavior.

  • src/components/pill-cloud/PillCloud.tsx#L27-L37: merge buttonProps.className and buttonProps.onClick, then keep the generated resin target.
  • src/components/pill-cloud/PillCloud.js.flow#L16-L27: apply the same prop ordering and callback-merging behavior.
📍 Affects 2 files
  • src/components/pill-cloud/PillCloud.tsx#L27-L37 (this comment)
  • src/components/pill-cloud/PillCloud.js.flow#L16-L27
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/pill-cloud/PillCloud.tsx` around lines 27 - 37, Protect
internally controlled props in both PillCloud implementations: update PillCloud
in src/components/pill-cloud/PillCloud.tsx (lines 27-37) and
src/components/pill-cloud/PillCloud.js.flow (lines 16-27) so buttonProps cannot
replace the generated pill classes, onSelect behavior, or resin target. Merge
buttonProps.className with the generated className, merge its onClick with the
internal selection callback, and place buttonProps before the generated props so
data-resin-target remains controlled.

>
{option.displayText}
</Button>
))}
</div>
);

export default PillCloud;
15 changes: 3 additions & 12 deletions ...ts/pill-cloud/__tests__/PillCloud.test.js → ...s/pill-cloud/__tests__/PillCloud.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react';
import { shallow } from 'enzyme';

import PillCloud from '..';

Expand Down Expand Up @@ -45,12 +46,7 @@ describe('components/pill-cloud/PillCloud', () => {
];
const wrapper = shallow(<PillCloud options={pills} selectedOptions={[pills[1]]} />);
const buttons = wrapper.find('.bdl-PillCloud-button');
expect(
buttons
.at(1)
.props()
.className.includes('is-selected'),
).toBeTruthy();
expect(buttons.at(1).props().className.includes('is-selected')).toBeTruthy();
});

test('should add selected class to a selected pill when passed by value', () => {
Expand All @@ -61,12 +57,7 @@ describe('components/pill-cloud/PillCloud', () => {
];
const wrapper = shallow(<PillCloud options={pills} selectedOptions={[{ value: 3, displayText: 'Sir' }]} />);
const buttons = wrapper.find('.bdl-PillCloud-button');
expect(
buttons
.at(2)
.props()
.className.includes('is-selected'),
).toBeTruthy();
expect(buttons.at(2).props().className.includes('is-selected')).toBeTruthy();
});

test('should pass selected child through to onSelect', () => {
Expand Down
0 ...ts__/__snapshots__/PillCloud.test.js.snap → ...s__/__snapshots__/PillCloud.test.tsx.snap
View file
Open in desktop
File renamed without changes.
0 src/components/pill-cloud/index.js → src/components/pill-cloud/index.js.flow
View file
Open in desktop
File renamed without changes.
2 changes: 2 additions & 0 deletions src/components/pill-cloud/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { default } from './PillCloud';
export type { PillCloudOption, PillCloudProps } from './PillCloud';
Loading