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

Remove extra menu separators #11827

Merged
merged 4 commits into from Feb 5, 2018

Conversation

Projects
None yet
3 participants
@codebytere
Member

codebytere commented Feb 5, 2018

Removes leading and trailing menu separators and resolves #5869.

Tested manually on MacOS, Windows, and Linux

To-Do:

  • Add specs

codebytere added some commits Feb 5, 2018

@codebytere codebytere requested a review from electron/reviewers as a code owner Feb 5, 2018

return visibleItems.filter((el, idx, array) => {
const meetsDeleteConditions = !visiblePrev || idx === array.length - 1 || array[idx + 1].type === 'separator'
const toDelete = el.type === 'separator' && meetsDeleteConditions

This comment has been minimized.

@felixrieseberg

felixrieseberg Feb 5, 2018

Member

This is a little hard to scan. I think that a reduce would work wonders here.

This comment has been minimized.

@ckerr

ckerr Feb 5, 2018

Member

I agree with felix this is a little hard to scan. I think the reasons are:

  • meetsDeleteConditions and toDelete are confusingly named, e.g. how can you meet delete conditions if type isn't separator?
  • visibleItems requires a little thought before you can tell how the loop will behave in edge cases

I'm not thinking functionally enough to see how a reduce would work cleanly here and would love enlightenment on this, no snark intended 😄

After poking around with a few approaches, the clearest I came up with was (untested):

function removeExtraSeparators(items) {
  // remove invisible items 
  items = items.filter(e => e.visible !== false)
  // fold adjacent separators together
  items = items.filter((e, idx, arr) => e.type !== 'separator' || idx==0 || arr[idx-1].type!=='separator')
  // remove edge separators
  return items.filter((e, idx, arr) => e.type !== 'separator' || (idx!=0 && idx!=arr.length-1))
}

Marking as Request Changes for cleanup to this section, but the above code is just a suggestion, not a requirement

return visibleItems.filter((el, idx, array) => {
const meetsDeleteConditions = !visiblePrev || idx === array.length - 1 || array[idx + 1].type === 'separator'
const toDelete = el.type === 'separator' && meetsDeleteConditions

This comment has been minimized.

@ckerr

ckerr Feb 5, 2018

Member

I agree with felix this is a little hard to scan. I think the reasons are:

  • meetsDeleteConditions and toDelete are confusingly named, e.g. how can you meet delete conditions if type isn't separator?
  • visibleItems requires a little thought before you can tell how the loop will behave in edge cases

I'm not thinking functionally enough to see how a reduce would work cleanly here and would love enlightenment on this, no snark intended 😄

After poking around with a few approaches, the clearest I came up with was (untested):

function removeExtraSeparators(items) {
  // remove invisible items 
  items = items.filter(e => e.visible !== false)
  // fold adjacent separators together
  items = items.filter((e, idx, arr) => e.type !== 'separator' || idx==0 || arr[idx-1].type!=='separator')
  // remove edge separators
  return items.filter((e, idx, arr) => e.type !== 'separator' || (idx!=0 && idx!=arr.length-1))
}

Marking as Request Changes for cleanup to this section, but the above code is just a suggestion, not a requirement

assert.equal(menu.items.length, 3)
assert.equal(menu.items[0].label, 'a')
assert.equal(menu.items[1].label, 'b')
assert.equal(menu.items[2].label, 'c')

This comment has been minimized.

@ckerr

ckerr Feb 5, 2018

Member

Another good edge case to test for would be [ sep, sep, a, b, c, sep, sep ] -> [ a, b, c ]

@ckerr

ckerr approved these changes Feb 5, 2018

🎉

@ckerr ckerr merged commit 5240352 into master Feb 5, 2018

8 checks passed

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
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the remove-leading-trailing-seps branch Feb 5, 2018

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

Remove extra menu separators (electron#11827)
* add function to remove leading/trailing separators

* change const  name for clarity

* add spec to check filtered separators

* clean method and add edge case spec per review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment