Skip to content

Conversation

ypatadia-eightfold
Copy link
Collaborator

@ypatadia-eightfold ypatadia-eightfold commented May 1, 2025

SUMMARY:

Pill will visible in single/multiselect for smaller screens

GITHUB ISSUE (Open Source Contributors)

JIRA TASK (Eightfold Employees Only):

https://eightfoldai.atlassian.net/browse/ENG-134598

CHANGE TYPE:

  • Bugfix Pull Request
  • Feature Pull Request

TEST COVERAGE:

  • Tests for this change already exist
  • I have added unittests for this change

TEST PLAN:

Copy link

codesandbox-ci bot commented May 1, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

factory-droid bot commented May 1, 2025

Important

Solutions for GitHub Action Failures - 69bc886 - 1

Build

To resolve the test failures, please follow these steps:

  1. Update the snapshot for the Select component by running:

    yarn test -u
    
  2. Update the useMaxVisibleSections hook tests in src/hooks/useMaxVisibleSections.test.tsx. Add the maxPillWidth property to the expected results:

    expect(result.current).toEqual({
      width: 20,
      count: 1,
      filled: false,
      maxPillWidth: 144,
    });

    Make sure to update both failing tests.

Additional Details

The test failures are due to recent changes in the Select component and useMaxVisibleSections hook. A new maxPillWidth property has been added, which affects both the rendering of the Select component and the return value of the hook.

In the Select component (src/components/Select/Select.tsx), a new style property has been added to the Pills:

style={{
  visibility: index < count ? 'visible' : 'hidden',
  maxWidth: `${maxPillWidth}px`,
}}

In the useMaxVisibleSections hook (src/hooks/useMaxVisibleSections.ts), the maxPillWidth property has been added to the return object:

return {
  width: 0,
  count: 0,
  filled: false,
  maxPillWidth: 144,
};

These changes are likely intentional improvements to the component's functionality. After updating the tests, review the changes to ensure they're working as expected in your application.

Copy link
Contributor

@factory-droid factory-droid bot left a comment

Choose a reason for hiding this comment

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

PR Description:
The PR enhances pill visibility in the Select component by implementing responsive pill widths for different screen sizes. The useMaxVisibleSections hook now dynamically calculates pill widths based on container size and item count, with specific handling for single items and narrow containers. 1

Open in Factory.
Click For Summary of Files

Summary of Files

Files Modified

src/hooks/useMaxVisibleSections.ts
Enhanced pill width calculations in Select component for better responsiveness:

- Added dynamic pill width calculation based on container size and item count
- Implemented special handling for single items (40px) and narrow containers (45% of width)
- Added maxPillWidth constraint (100px for narrow containers, 144px for normal cases)
src/components/Select/Select.tsx
Enhanced pill visibility in Select component with responsive width constraints:

- Modified useMaxVisibleSections hook integration to handle dynamic pill widths
- Reduced default pill width parameter from 168px to 144px
- Added maxWidth constraint to individual pills for consistent sizing
Tips
Review Droid is highly customizable and comes with powerful features for augmenting your organization's code review process. Here are some tips to get the most out of it.

Table of contents

⌨️ Droid Fill

Contextual PR Body Replacement

When you create a PR with the @droid fill command anywhere in your PR body, Review Droid will fill in the PR description for your pull request based on it's PR analysis. This will also take into account your pull request templates.

Review Droid can also analyze your project management system. If you have a project management system integrated with Factory (e.g. Linear, Jira) Review Droid will also integrate information from linked and related tickets.

At Factory, we typically create our PR's with this command. For example, let's say I'm creating a PR which addresses the jira ticket FAC-123. I would write the following PR description:

@droid fill FAC-123

and your Review Droid fills in the rest!

📚 Review Guidelines

Creating guidelines for Droid to follow

You can configure guidelines that Droid will follow when reviewing your PRs. Droid will focus on these aspects of your code and aim to leave in-line comments if any guidelines are violated.

Guidelines are defined in your repository's .droid.yaml. Every week, Droid will automatically refine and edit these guidelines based on the feedback you leave on Droid's comments.

💬 Droid Chat

Ask questions on a PR

You can leave in-line comments on PR's by tagging @droid in-line. This can be helpful when reviewing other's PRs. Some examples include:

  • @droid this section looks sketchy, are there issues with it?
  • @droid can you show me some examples of what this regex matches?
  • @droid is this the most efficient way to do this? I'm concerned about performance.

Follow up with Review Droid's Comments

You can reply to Review Droid's in-line review comments directly to ask questions or provide feedback. Some examples include:

  • @droid I made the change you suggested, does that fix the issue?
  • @droid we don't actually need to do this because of X, Y, Z. Can you confirm?
  • @droid do we have any scripts that rely on this behavior?

🛠️ PR Healing

Diagnose & Fix Failures in CI

Review Droid is aware of the CI processes you utilize and proposes fixes in case of any failures. This allows it to promptly address issues in your pull requests before they escalate.

By default, PR Healing is activated. Your organization does not have advanced PR healing enabled, which involved Review Droid directly making a PR to your PR which fixes the issue. If you would like to enable this feature, you must have an Enterprise Plan.

🎓 Teaching Droid

Giving Droid feedback so it learns

You can give feedback to Review Droid by replying or reacting to its comments (👍 / 👎). This helps Review Droid learn from your preferences and improve its future reviews.

To send feedback directly to the Factory team, include @droid feedback in your comment. Droid will file a ticket with your feedback and provide a ticket ID so you can track it with our support team.

🔎 Review Usage

Re-Requesting Review

If you make changes to your PR and want Review Droid to re-review it, you can simply comment @droid review on the PR. This will trigger Droid to re-review the PR and update the review body.

.droid.yaml to Configure Review Droid

You can place a .droid.yaml file in the root of your repository. This file contains settings for a variety of features and settings including:

  • Guidelines - For defining the rules that Review Droid will enforce
  • Enabling/Disabling Per-file Summaries
  • Enabling/Disabling PR Healing
  • Path Filters (For ignoring certain files or directories)
  • Auto-Review Settings
  • Chat settings

To override a setting leave a comment on a PR with the setting to disable/enable/reset. For example @droid setting disable progress_comment. The current options are: progress_comment, lgtm_comment, and list.

list is a special setting that will list all the settings that you have set and will explain what each setting does.

For more information, you can view our documentation at https://docs.factory.ai - the password is factory.

Ignoring Reviews

If you want to have your PR ignored by Review Droid you can define Droid Ignored Title Words in your .droid.yaml file. If the title of your PR contains any of these words, Review Droid will ignore the PR.

Your organization currently has the following words in the Droid Ignored Title Words list:
None

Footnotes

  1. React 👍 / 👎 to comments you like / dislike. Droid will learn from this feedback to leave better reviews. My in-line comments are based on your review guidelines. If you have any questions about the comments, reply to them with @droid

Comment on lines +21 to +22
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) return;
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
const selectedItemsLength = itemsRef.current?.filter(Boolean)?.length;
if (selectedItemsLength === 0) return;
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.
    "

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 { 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

Comment on lines +23 to +38
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;
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants