Skip to content

fix: properly apply minwidth set by Tableview column menu #9838

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

Merged
LFDanLu merged 1 commit into from
Mar 27, 2026
+1 −1
Merged

fix: properly apply minwidth set by Tableview column menu #9838

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
2 changes: 1 addition & 1 deletion packages/@react-spectrum/s2/src/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ export const Menu = /*#__PURE__*/ (forwardRef as forwardRefType)(function Menu<T
hideArrow>
<div
style={UNSAFE_style}
className={(UNSAFE_className || '') + wrappingDiv}>
className={(UNSAFE_className || '') + mergeStyles(wrappingDiv, styles)}>
Copy link
Copy Markdown
Member Author

@LFDanLu LFDanLu Mar 25, 2026

Choose a reason for hiding this comment

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

react-spectrum/packages/@react-spectrum/s2/src/TableView.tsx

Line 825 in a911b98

<Menu onAction={onMenuSelect} styles={style({minWidth: 128})}>

The above min-width wasn't making it through, resulting in cases like:

Image

Still trying to get the exact reproduction details, but it felt like said min width (and other user provided menu styles) should make it through to the Popover since they don't get added to the Menu in this case

Copy link
Copy Markdown
Member

@snowystinger snowystinger Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

This would mean that styles are duplicated on AriaMenu and on wrapper div

But I think we actually want the minWidth (and other styles) on the Popover? And only applied isPopover is true?

Copy link
Copy Markdown
Member Author

@LFDanLu LFDanLu Mar 25, 2026

Choose a reason for hiding this comment

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

we skip applying the styles on the menu if isPopover is true, hence why on prod the minWidth wasn't making it through already:

react-spectrum/packages/@react-spectrum/s2/src/Menu.tsx

Line 397 in a911b98

className={menu({size, isPopover}, isPopover ? null : styles)}>

the above change is also within a isPopover branch (hard to see from just the diff) so there shouldn't be a duplication I believe:

react-spectrum/packages/@react-spectrum/s2/src/Menu.tsx

Lines 404 to 417 in a911b98

if (isPopover) {
return (
<Popover
ref={ref}
padding="none"
hideArrow>
<div
style={UNSAFE_style}
className={(UNSAFE_className || '') + wrappingDiv}>
{content}
</div>
</Popover>
);
}

Copy link
Copy Markdown
Member

@snowystinger snowystinger Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

gah i thought i'd checked that but my brain missed line 404 somehow

still a bit weird it goes on the div inside the popover and isn't applied to the Popover itself, with our changes to Popover, maybe that extra div is unnecessary now?

Copy link
Copy Markdown
Member Author

@LFDanLu LFDanLu Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I don't quite remember all the details but this was attempted prior to f9fc374#diff-0c586b0d4f573495a7818f614c64cb7293dd6d34ebdbce6f92f3dcd72009a073, but then we decided return to clamping down on allowed Popover style overrides hence bringing back the inner div.

The approach used in Menu falls in line with "as per discussion with team, if someone wants to modify their popover internals, they are expected to add a inner wrapping div themselves and turn off padding like custom dialog"

Copy link
Copy Markdown
Member

@snowystinger snowystinger Mar 25, 2026

Choose a reason for hiding this comment

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

Thanks for the history again

{content}
</div>
</Popover>
Expand Down
Loading