8000 feat: add menu item role `palette` and `header` by gerhardberger · Pull Request #45538 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add menu item role palette and header #45538

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

Jump to bottom
Merged
merged 5 commits into from
May 23, 2025
Merged

feat: add menu item role palette and header #45538

merged 5 commits into from
May 23, 2025

Conversation

gerhardberger
Copy link
Contributor
@gerhardberger gerhardberger commented Feb 9, 2025 •
edited
Loading

Description of Change

Closes #42960

This PR adds two new macOS specific menu item roles:

  • palette: A menu presentation style where items to display align horizontally.
  • header: A menu item representing a section header for a logical grouping of menu commands.
const reactions = []

  for (let i = 1; i <= 4; ++i) {
    reactions.push({
      label: '',
      icon: nativeImage.createFromPath(path.resolve(__dirname, '..', `reaction-${i}@2x.png`)),
      click: () => console.log('Clicked reaction!')
    })
  }

  const paletteMenu = Menu.buildFromTemplate(reactions)

  const contextMenu = Menu.buildFromTemplate([
    {
      label: 'Reactions',
      role: 'header',
    },
    {
      type: 'submenu',
      label: 'Reactions',
      role: 'palette',
      submenu: paletteMenu,
    },
    {
      type: 'submenu',
      label: 'Reactions',
      submenu: paletteMenu,
    },
    { label: 'Tapback...' },
    { label: 'Reply...' },
    { label: 'Add Sticker...' },
  ])
image

Checklist

Release Notes

Notes: Add support for menu item role palette and header on macOS.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Feb 9, 2025
@jkleinsc jkleinsc added the semver/minor backwards-compatible functionality label Feb 10, 2025
jkleinsc
jkleinsc reviewed Feb 10, 2025
View reviewed changes
Copy link
Member
@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

samuelmaddock
samuelmaddock approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock 8000 left a comment

Choose a reason for hiding this comment

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

API LGTM

erickzhao
erickzhao approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member
@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

API LGTM

@Magnate1234 Magnate1234 mentioned this pull request Feb 11, 2025
Closed
2 tasks
@codebytere
Copy link
Member

@gerhardberger could you please sign your commits?

@gerhardberger gerhardberger force-pushed the feat-add-palette-header-menu branch 2 times, most recently from d9f897e to e4f8059 Compare February 12, 2025 10:31
itsananderson
itsananderson reviewed Feb 13, 2025
View reviewed changes
Copy link
Member
@itsananderson itsananderson left a comment

Choose a reason for hiding this comment

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

Can you add the code sample to the PR details that you used to generate the screenshot with the palette? It's a little hard to tell how that role would be used based on the docs, so that would be helpful for reviewing the API.

samuelmaddock
samuelmaddock requested changes Feb 20, 2025
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API CHANGES REQUESTED

After further discussion we came to the conclusion that the role property might not be the best fit for these additions. We recommend using the type property instead, partially based on findings that the 'separator' type uses similar code.

@bpasero
Copy link
Contributor
bpasero commented Mar 27, 2025

Is this going on?

@gerhardberger
Copy link
Contributor Author

@bpasero yes I'm planning to wrap it up with the requested changes in the coming weeks.

@gerhardberger
Copy link
Contributor Author

@samuelmaddock I looked into the requested change to implement it with the menu's type property and it would be a little convoluted because we cannot just extend the electron::ElectronMenuModel::ItemType with the new types, because that type is coming from Chromium's ui::MenuModel.
So IMO what we could do is implement it as an extra CustomType field next to ItemType, store them in ElectronMenuModel and handle them accordingly in electron_menu_controller.mm.
If this sounds good, I can go ahead with it.

@samuelmaddock
Copy link
Member

@gerhardberger That approach seems reasonable 👍

@gerhardberger gerhardberger force-pushed the feat-add-palette-header-menu branch from cecf5f5 to 9642f49 Compare May 20, 2025 18:37
@erickzhao erickzhao requested a review from samuelmaddock May 20, 2025 18:53
@erickzhao erickzhao removed the wip ⚒ label May 20, 2025
itsananderson
itsananderson reviewed May 22, 2025
View reviewed changes
Copy link
Member
@itsananderson itsananderson left a comment

Choose a reason for hiding this comment

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

I like the new approach! Just a couple comments about documenting the new types as macOS 14+ only.

samuelmaddock
samuelmaddock approved these changes May 22, 2025
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

refactor lgtm!

samuelmaddock
samuelmaddock approved these changes May 22, 2025
View reviewed changes
Copy link
Member
@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

itsananderson
itsananderson approved these changes May 22, 2025
View reviewed changes
Copy link
Member
@itsananderson itsananderson left a comment

Choose a reason for hiding this comment

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

API LGTM

@samuelmaddock samuelmaddock added the target/37-x-y PR should also be added to the "37-x-y" branch. label May 23, 2025
@samuelmaddock samuelmaddock force-pushed the feat-add-palette-header-menu branch from b482dc6 to 6495d82 Compare 9E88 May 23, 2025 13:50
@jkleinsc jkleinsc merged commit b9b96a9 into electron:main May 23, 2025
55 checks passed
@release-clerk
Copy link
release-clerk bot commented May 23, 2025

Release Notes Persisted

Add support for menu item role palette and header on macOS.

@trop trop bot mentioned this pull request May 23, 2025
Open
@trop
Copy link
Contributor
trop bot commented May 23, 2025

I have automatically backported this PR to "37-x-y", please check out #47245

@trop trop bot added in-flight/37-x-y and removed target/37-x-y PR should also be added to the "37-x-y" branch. labels May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsananderson itsananderson itsananderson approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@samuelmaddock samuelmaddock samuelmaddock approved these changes

@erickzhao erickzhao erickzhao approved these changes

Assignees
No one assigned
Labels
api-review/approved ✅ in-flight/37-x-y semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Support macOS palette menus
8 participants
@gerhardberger @codebytere @bpasero @samuelmaddock @itsananderson @jkleinsc @erickzhao @deepak1556
0