Skip to content

fix: pill visibility on small screens #991

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
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: pill visibility on small screens #991

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69bc886
fix: pill visibility on small screens
ypatadia-eightfold May 1, 2025
c6a2c5e
Merge branch 'main' into ypatadia/select-mobile-pill-fix/ENG-134598
ypatadia-eightfold Jun 12, 2025
53d77b3
Merge branch 'main' into ypatadia/select-mobile-pill-fix/ENG-134598
ypatadia-eightfold Jul 18, 2025
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
5 changes: 3 additions & 2 deletions src/components/Select/Select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,10 @@ export const Select: FC<SelectProps> = React.forwardRef(
return (options || []).filter((option: SelectOption) => option.selected);
};

const { count, filled, width } = useMaxVisibleSections(
const { count, filled, width, maxPillWidth } = useMaxVisibleSections(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why maxPillWidth, just pillWidth

inputRef,
pillRefs,
168,
144,
8,
1,
getSelectedOptionValues().length
Expand Down Expand Up @@ -590,6 +590,7 @@ export const Select: FC<SelectProps> = React.forwardRef(
type={readonly ? PillType.default : PillType.closable}
style={{
visibility: index < count ? 'visible' : 'hidden',
maxWidth: `${maxPillWidth}px`,
}}
{...pillProps}
/>
Expand Down
24 changes: 22 additions & 2 deletions src/hooks/useMaxVisibleSections.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,37 @@ export const useMaxVisibleSections = (
width: 0,
count: 0,
filled: false,
maxPillWidth: 144,
Copy link
Contributor

Choose a reason for hiding this comment

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

1 Define an interface for the maxSections object shape to improve type safety and maintainability:

interface MaxSections {
  readonly width: number;
  readonly count: number;
  readonly filled: boolean;
  readonly maxPillWidth: number;
}

Then use this interface consistently:

const [maxSections, setMaxSections] = useState<MaxSections>({...});

Footnotes

  1. This suggestion is based on your review guideline: "Follow our TypeScript style guide. Use interfaces for object shapes and types for unions/primitives. Prefer readonly properties when values shouldn't change. Use proper enums for function parameters and return types. Avoid using 'any' type when possible, prefer unknown for truly unknown types. Use type guards to narrow types safely. Leverage TypeScript's utility types (Partial, Required, Pick, etc.) when appropriate.
    "
    External URL: Use interfaces for object shapes and types for unions/primitives ↩

});

const computeVisibleSections = () => {
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) return;
Comment on lines +21 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

1 Early return should maintain consistent object shape to prevent runtime errors:

if (selectedItemsLength === 0) {
  return {
    width: 0,
    count: 0,
    filled: false,
    maxPillWidth: 144
  };
}
Committable suggestion

Please carefully review the code before committing.

Suggested change
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) return;
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) {
return {
width: 0,
count: 0,
filled: false,
maxPillWidth: 144
};
}

Footnotes

  1. This suggestion is based on your review guideline: "Catch and correct any obvious bugs. Ensure code is clean, maintainable, and follows project conventions. Check for security vulnerabilities and performance issues. Avoid circular dependencies.
    " ↩

Comment on lines +21 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

1 Return consistent object shape when no items are selected:

if (selectedItemsLength === 0) {
  return {
    width: 0,
    count: 0,
    filled: false,
    maxPillWidth: 144
  };
}
Committable suggestion

Please carefully review the code before committing.

Suggested change
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) return;
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) {
return {
width: 0,
count: 0,
filled: false,
maxPillWidth: 144
};
}

Footnotes

  1. This suggestion is based on your review guideline: "Add 'use client' directive at the top of client-side hooks. Export hooks as named exports. Use TypeScript generics for type-safe hooks when appropriate. Add JSDoc comments to describe hook functionality. Return consistent values from hooks. Follow React hooks rules (only call hooks at the top level, only call hooks from React functions). Handle cleanup functions properly in useEffect to prevent memory leaks. Use dependency arrays correctly in useEffect, useMemo, and useCallback. Create custom hooks for reusable logic across components. Ensure hooks have proper error handling and edge case management.
    " ↩

const containerWidth =
containerRef.current?.getBoundingClientRect().width ?? 0;
const firstItemWidth =
itemsRef.current?.[0]?.getBoundingClientRect().width ?? extraItemWidth;

const isSingleItem = selectedItemsLength === 1;
const isNarrowContainer = isSingleItem
? containerWidth <= 210
: containerWidth <= 350;

const updatedExtraItemWidth = isSingleItem
? 40
: isNarrowContainer
? containerWidth * 0.45
: firstItemWidth + 24;
const maxPillWidth = isNarrowContainer ? 100 : 144;
Comment on lines +23 to +38
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this possible without doing a boundingClientRect call

const availableWidth: number =
(containerRef.current?.getBoundingClientRect().width - extraItemWidth) *
linesToShow;
(containerWidth - updatedExtraItemWidth) * linesToShow;
const sections = itemsRef.current?.reduce(
(
acc: {
width: number;
count: number;
filled: boolean;
maxPillWidth: number;
},
ref: HTMLElement
) => {
Expand All @@ -45,6 +64,7 @@ export const useMaxVisibleSections = (
width: 0,
count: 0,
filled: false,
maxPillWidth: maxPillWidth,
}
);
setMaxSections(sections);
Expand Down
Loading