feat: limit badges to 2 with hover/click popover in UserListTable #26556
+250
−150
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
…bile Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
devin-ai-integration
bot
changed the title
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
devin-ai-integration
bot
changed the title
- Refactor LimitedBadges to concrete component with BadgeItem type - Move exports to end of files to comply with useExportsLast rule - Add explicit types to callback parameters for useExplicitType rule - Convert nested ternary to if-else for filterType calculation - Remove unused imports (Row, Table types) - Update ResponseValueCell and UserListTable to use new LimitedBadges API Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Add FilterType import from @calcom/types/data-table - Add FacetedValue import from @calcom/features/data-table - Type filterType as FilterType to allow reassignment to different ColumnFilterType values - Type getFacetedUniqueValues return as Map<FacetedValue, number> Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Contributor
Author
|
@cubic-dev-ai review this |
Contributor
@eunjae-lee I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/modules/insights/components/ResponseValueCell.tsx">
<violation number="1" location="apps/web/modules/insights/components/ResponseValueCell.tsx:20">
P2: Potential duplicate React keys if `values` array contains duplicate entries. The original code used the array index `i` in the key to ensure uniqueness (`${cellId}-${i}-${rowId}`), but the new code only uses `${id}-${rowId}`. Include the index to preserve the original uniqueness guarantee.</violation>
</file>
<file name="apps/web/modules/users/components/UserTable/UserListTable.tsx">
<violation number="1" location="apps/web/modules/users/components/UserTable/UserListTable.tsx:263">
P2: Regression: Group option badges lose their distinctive orange styling. The original code used `variant={isAGroupOption ? "orange" : "gray"}` to visually distinguish group options, but this is now hardcoded to `"gray"`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Add index to id for unique keys in ResponseValueCell.tsx - Restore orange variant for group options in UserListTable.tsx - Fix useMemo dependency array (add t, remove dispatch) - Fix import ordering with biome - Convert ternary operators to if-else statements for biome compliance Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
- Remove id field from BadgeItem type in LimitedBadges - Use label as React key instead of id - Remove unused rowId parameter from ResponseValueCell - Update all consumers to not pass id field Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
Contributor
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/components/ui/LimitedBadges.tsx">
<violation number="1" location="apps/web/components/ui/LimitedBadges.tsx:69">
P2: Using `index` as key instead of `label`. The PR description states the component uses `label` as React key, but the implementation uses `index`. Consider using `item.label` as key to match the documented behavior and avoid potential reconciliation issues when items change.</violation>
<violation number="2" location="apps/web/components/ui/LimitedBadges.tsx:94">
P2: Using `index` as key for hidden items. Consider using `item.label` as key for consistency with the documented behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <div className="flex flex-col gap-1"> | ||
| {hiddenItems.map((item, index) => ( | ||
| <span | ||
| key={index} |
Contributor
There was a problem hiding this comment.
P2: Using index as key for hidden items. Consider using item.label as key for consistency with the documented behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/ui/LimitedBadges.tsx, line 94:
<comment>Using `index` as key for hidden items. Consider using `item.label` as key for consistency with the documented behavior.</comment>
<file context>
@@ -0,0 +1,109 @@
+ <div className="flex flex-col gap-1">
+ {hiddenItems.map((item, index) => (
+ <span
+ key={index}
+ className="text-default cursor-pointer text-sm hover:text-emphasis"
+ onClick={item.onClick}>
</file context>
Suggested change
| key={index} | |
| key={item.label} |
| return ( | ||
| <div className={`flex flex-wrap items-center gap-x-1 gap-y-1 ${className || ""}`}> | ||
| {visibleItems.map((item, index) => ( | ||
| <Badge key={index} variant={item.variant || "gray"} onClick={item.onClick}> |
Contributor
There was a problem hiding this comment.
P2: Using index as key instead of label. The PR description states the component uses label as React key, but the implementation uses index. Consider using item.label as key to match the documented behavior and avoid potential reconciliation issues when items change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/ui/LimitedBadges.tsx, line 69:
<comment>Using `index` as key instead of `label`. The PR description states the component uses `label` as React key, but the implementation uses `index`. Consider using `item.label` as key to match the documented behavior and avoid potential reconciliation issues when items change.</comment>
<file context>
@@ -0,0 +1,109 @@
+ return (
+ <div className={`flex flex-wrap items-center gap-x-1 gap-y-1 ${className || ""}`}>
+ {visibleItems.map((item, index) => (
+ <Badge key={index} variant={item.variant || "gray"} onClick={item.onClick}>
+ {item.label}
+ </Badge>
</file context>
Suggested change
| <Badge key={index} variant={item.variant || "gray"} onClick={item.onClick}> | |
| <Badge key={item.label} variant={item.variant || "gray"} onClick={item.onClick}> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Limits the number of badges displayed in the "teams" column and attribute columns in the UserListTable to a maximum of 2. When there are more than 2 badges, only the first 2 are shown with a "+N" badge that opens a popover to display all hidden items.
Changes
New Component:
LimitedBadgesCreated a reusable
LimitedBadgescomponent inapps/web/components/ui/LimitedBadges.tsxthat:BadgeItemobjects (label, variant, onClick)maxVisiblebadges (default: 2) with a "+N" indicator for overflowuseMediaQueryto detect device type)useMemoanduseCallbackfor performance optimizationidfield required)UserListTable Updates
LimitedBadgesto limit displayed team badgesLimitedBadgeswith a simplified label format showing value, weight percentage, and group indicator (e.g., "Value 100% (group)")ResponseValueCell Updates
LimitedBadgescomponent instead of inline badge renderingrowIdparameter (no longer needed since array index is used as key)Demo
Updates since last revision
idfield fromBadgeItemtype - component now uses array index as the React key for simplicityrowIdparameter fromResponseValueCellcomponentMandatory Tasks (DO NOT REMOVE)
How should this be tested?
Human Review Checklist
ResponseValueCellin insights module still works correctly (rowId parameter was removed)Checklist
Link to Devin run
https://app.devin.ai/sessions/f4de6199ef3841b884221abf6e69a62a
Requested by
@eunjae-lee (eunjae@cal.com)