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

Add menu item order control #12362

Merged
merged 16 commits into from May 5, 2018

Conversation

4 participants
@codebytere
Member

codebytere commented Mar 20, 2018

This PR reworks the way that we approach menu item ordering by porting an approximation of the solution devised in atom/atom with atom/atom#16661 to Electron.

Fixes #11668.

To-Do:

  • Fix two failing tests
  • Documentation

This PR would hopefully be a breaking change in 3-0-x.

@codebytere codebytere requested a review from electron/reviewers as a code owner Mar 20, 2018

@codebytere codebytere changed the title from Add context menu order control to Add menu item order control Mar 20, 2018

}
])
assert.equal(menu.items[0].label, '1')
assert.equal(menu.items[1].label, '2')
assert.equal(menu.items[2].label, '3')
})
it('should position after existing item', () => {
it.only('should position after existing item', () => {

This comment has been minimized.

@codebytere

codebytere Mar 25, 2018

Member

i need to remove this i know; i'll do it with the next commit

@codebytere codebytere requested a review from electron/docs as a code owner Mar 25, 2018

@@ -0,0 +1,169 @@
function splitArray (arr, predicate) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 24, 2018

Member

I really want this to be a reduce (not a fan of a forEach creating a new array)

const result = arr.reduce((multi, item) => {
  const current = multi[multi.length - 1]
  if (predicate(item)) {
    if (current.length > 0) multi.push([])
  } else {
    current.push(item)
  }
  return multi
}, [[]])
if (result[result.length - 1].length === 0) {
  return result.slice(0, result.length - 1)
}
return result

Not sure if that's ^^ the best way to do it but would love to see the forEach go away 😄

return multiArr
}
function joinArrays (arrays, joiner) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Apr 24, 2018

Member

This is better as a reduce

return arrays.reduce((joined, arr, i) => {
  if (i > 0 && arr.length > 0) {
    joined.push(joiner)
  }
  return joined.concat(arr)
}, []);

@codebytere codebytere added this to Candidates in 3.0.x Apr 24, 2018

@zeke

Left a few comments.

I find the afterGroupContaining and beforeGroupContaining descriptions difficult to understand. Can you take a shot at rewording them?

It might help to have some illustrative examples. Maybe it's high time we had a Menu tutorial 🤔

function joinArrays (arrays, joiner) {
const joinedArr = []
arrays.forEach((arr, i) => {
if (i > 0 && arr.length > 0) {

This comment has been minimized.

@zeke

zeke May 2, 2018

Member

0 is falsy, so you could just do && arr.length

This comment has been minimized.

@zeke

zeke May 2, 2018

Member

But this is more explicit. I don't care! Just a note.

@@ -302,27 +302,20 @@ browser windows.
## Menu Item Position
You can make use of `position` and `id` to control how the item will be placed
when building a menu with `Menu.buildFromTemplate`.
You can make use of `before/after`, `beforeGroupContaining/afterGroupContaining` and `id` to control how the item will be placed when building a menu with `Menu.buildFromTemplate`.

This comment has been minimized.

@zeke

zeke May 2, 2018

Member

I would break out before/after into before, after

an existing item in the menu:
* `before` - Inserts this item before the id referenced item. If the
* `before` - Inserts this item before the label referenced item. If the

This comment has been minimized.

@zeke

zeke May 2, 2018

Member

"before the label referenced item"? Not sure I understand this.

This comment has been minimized.

@codebytere

codebytere May 4, 2018

Member

revised to after the item with the specified label

the first item.
the menu. Also implies that the menu item in question should be placed in the same “group” as the item
* `beforeGroupContaining` - Provides a means for a single context menu to declare
the placement of their containing group after to the containing group of the label specified item.

This comment has been minimized.

@zeke

zeke May 2, 2018

Member

typo: "after to the containing"

* `afterGroupContaining` Provides a means for a single context menu to declare
the placement of their containing group before the containing group of the label specified item.
By default, items will be inserted in the order they exist in the template unless one of the specified positioning keywords is utilized.

This comment has been minimized.

@zeke

zeke May 2, 2018

Member

utilized => used

an existing item in the menu:
* `before` - Inserts this item before the id referenced item. If the
* `before` - Inserts this item before the item with the specified label.. If the

This comment has been minimized.

@zeke

zeke May 4, 2018

Member

typo: ..

the first item.
the menu. Also implies that the menu item in question should be placed in the same “group” as the item.
* `beforeGroupContaining` - Provides a means for a single context menu to declare
the placement of their containing group after the containing group of the item with the specified label.

This comment has been minimized.

@zeke

zeke May 4, 2018

Member

s/after/before/

}
function attemptToMergeAGroup (groups) {
for (let i = 0; i < groups.length; i++) {

This comment has been minimized.

@zeke

zeke May 4, 2018

Member

i can be a const here

This comment has been minimized.

@codebytere

codebytere May 4, 2018

Member

won't that not let me increment i?

This comment has been minimized.

@zeke

zeke May 5, 2018

Member

derp sorry

@zeke

zeke approved these changes May 5, 2018

codebytere added some commits Mar 20, 2018

@codebytere codebytere merged commit 9c8952a into master May 5, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

3.0.x automation moved this from Candidates to Fixed May 5, 2018

@codebytere codebytere deleted the context-menu-order-control branch May 5, 2018

@ckerr ckerr moved this from Fixed to Fixed (3.0.0-beta.1) in 3.0.x Jul 12, 2018

MarshallOfSound added a commit to electron/electron-typescript-definitions that referenced this pull request Sep 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment