Skip to content
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

Removing unnecessary menu items from the address bar field context menu #384

Merged
merged 4 commits into from
Jan 11, 2022

Conversation

tomasstrba
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/1148564399326804/1201546959960286

Description:
There was unnecessary "Make Link", "Search with Google" and "Fonts" menu items in the context menu of the address bar text field.

Steps to test this PR:

  1. Load a website,
  2. Unfocus the address bar
  3. Right click the address bar

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

@tomasstrba
Copy link
Contributor Author

tomasstrba commented Dec 22, 2021

Note: I will remove "Search with Google" right after my Xcode is updated

@bstandaert-ddg
Copy link

@tomasstrba Maybe the entire bottom section of the menu should be removed? None of the actions in it seem very useful for the address bar, and several of the items don't work or make the address bar layout break (spelling and grammar > show spelling and grammar, subsitutions > data detectors, layout orientation > vertical).

@tomasstrba tomasstrba force-pushed the tom/menu-item-removal branch from dd0b700 to da1cf9e Compare December 23, 2021 21:16
@tomasstrba
Copy link
Contributor Author

Ben, agree. I removed everything except Services since it is managed elsewhere. Please see Services menu
I am going to track another issue for it because it also contains "Search with Google" item.

@tomasstrba tomasstrba force-pushed the tom/menu-item-removal branch from da1cf9e to d5cd41b Compare December 25, 2021 12:30
Copy link
Collaborator

@mallexxx mallexxx left a comment

Choose a reason for hiding this comment

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

LGTM

private func removingAttributeChangingMenuItems(from menu: NSMenu) -> NSMenu {
menu.items.reversed().forEach { menuItem in
if let action = menuItem.action {
if Self.selectorsToRemove.contains(action) { menu.removeItem(menuItem) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

also probably need to return after the If

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to create else branch to make it more clear

Copy link
Collaborator

@mallexxx mallexxx Jan 11, 2022

Choose a reason for hiding this comment

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

yes, kinda.. May you also consider replacing the .forEach() with for each menuItem? This would mean continue usage 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

continue and remove combination can be a bit missleading. How about now?

return removingAttributeChangingMenuItems(from: menu)
}

private static var selectorsToRemove = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's replace it with a Set

@tomasstrba tomasstrba merged commit ccbd795 into develop Jan 11, 2022
@tomasstrba tomasstrba deleted the tom/menu-item-removal branch January 11, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants