Skip to content

chore(app): Improve Table column header menu positioning and styling #4012

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.

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
merged 5 commits into from
Apr 7, 2025

Conversation

domphan-wandb
Copy link
Contributor

@domphan-wandb domphan-wandb commented Mar 31, 2025

Description

JIRA: WB-24222

This PR improves the styling of the column action menu:

  1. Reversed the order so that the icon comes before the action description
  2. Slightly darkened the column header when its action menu is open
  3. Changed the background color from black to white.
  4. Lightened the divider color to reduce contrast
  5. Reduced the number of dividers to separate for 3 categories:
    a. editing actions (edit cell expr)
    b. organization actions (group, sort, pin)
    c. column manipulation actions (insert, remove)

I had to bring over weave-js/src/common/components/WBPopupMenuTrigger.tsx from wandb/ui (no longer built for app) to update the ordering of the icon and text by passing in an optionRenderer.

Design doc used as reference: https://www.notion.so/wandbai/RFC-Query-panels-Table-usability-improvements-Draft-1a4e2f5c7ef380cf8d3fd7c28c51b992?pvs=4

image

Testing

How was this PR tested?

@domphan-wandb domphan-wandb changed the title Dom/column menu design feat(app): Improve Table column header menu positioning and styling Mar 31, 2025
@domphan-wandb domphan-wandb force-pushed the dom/column-menu-design branch from c84a17e to 5f253fe Compare April 3, 2025 00:10
@domphan-wandb domphan-wandb changed the title feat(app): Improve Table column header menu positioning and styling chore(app): Improve Table column header menu positioning and styling Apr 3, 2025
@domphan-wandb domphan-wandb marked this pull request as ready for review April 3, 2025 15:34
@domphan-wandb domphan-wandb requested review from a team as code owners April 3, 2025 15:34
@domphan-wandb domphan-wandb force-pushed the dom/column-menu-design branch 4 times, most recently from 96173f4 to 7657231 Compare April 3, 2025 17:10
@domphan-wandb domphan-wandb force-pushed the dom/column-menu-design branch 2 times, most recently from 155e073 to 67380e2 Compare April 4, 2025 22:05
@domphan-wandb domphan-wandb force-pushed the dom/column-menu-design branch from 67380e2 to c912d15 Compare April 4, 2025 22:15
<ItemIcon
style={{marginRight: '8px', marginLeft: 0}}
name={
option.icon ?? (selected && option.icon !== null ? 'check' : 'blank')
Copy link
Contributor

@onx2 onx2 Apr 7, 2025

Choose a reason for hiding this comment

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

can this be selected && option.icon ? 'check' : 'blank'? NVM I see what youre doing.

Maybe this can be clearer though

Comment on lines 569 to 572
const handleOpenChange = useCallback((open: boolean) => {
setMenuOpen(open);
}, []);

Copy link
Contributor

Choose a reason for hiding this comment

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

useCallback doesn't seem necessary here, have you tested this?

@domphan-wandb
Copy link
Contributor Author

Thanks @onx2 ! I updated the PR to address your comments:

  • Removed the unnecessary useCallback. The functionality works and was tested manually/visually (col header has a slight grey when menu is opened)
  • Simplified the conditional logic to handle no icon option being passed in

@domphan-wandb domphan-wandb requested a review from onx2 April 7, 2025 15:34
@domphan-wandb domphan-wandb merged commit 3217e00 into master Apr 7, 2025
138 of 140 checks passed
@domphan-wandb domphan-wandb deleted the dom/column-menu-design branch April 7, 2025 18:13
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants