Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Disabled context menu items for unsaved files #12806

Merged

Conversation

mansimarkaur
Copy link
Contributor

Fixes #11763

@ficristo
Copy link
Collaborator

Have you checked what other editors do in these cases?
In general you need some jsdocs.
@petetnt @zaggino could someone of you review?

Copy link
Collaborator

@petetnt petetnt left a comment

Choose a reason for hiding this comment

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

LGTM sans couple of missing pieces of JSDoc

MainViewManager = require("view/MainViewManager"),
CommandManager = require("command/CommandManager");

function _setContextMenuItemsVisible(enabled, items) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs JSDoc

});
}

function _setMenuItemsVisible() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs JSDoc

@petetnt
Copy link
Collaborator

petetnt commented Sep 29, 2016

Thanks for this great PR @mansimarkaur! Tested against latest master and it works great.

Could you add few small pieces of JSDoc for the new methods and maybe simple unit test for the functionality? (it should disable context menu items when not present and it should not disable context menu items when present)

👍

@mansimarkaur
Copy link
Contributor Author

@petetnt sure, I'll add the required changes.

@mansimarkaur
Copy link
Contributor Author

@petetnt Could you please help me with the tests? I was thinking of adding tests in the test/Menu-test.js file. Please let me know if it's right?

@petetnt
Copy link
Collaborator

petetnt commented Oct 27, 2016

@mansimarkaur test/Menu-test.js sounds like a good place.

Any specific things you need help with the tests? The structure should could be something like:

it("it should disable context menu items when file doesn't exist ", function () {
  // runs create a new file
  // opens context menu
  // checks that all the items are disabled
})
it("it should enable context menu items when file does exist ", function () {
  // runs create a new file and saves it
  // opens context menu
  // checks that all the items are enabled
})

For example check DocumentCommandHandlers tests to see how the working set can be manipulated.

@Denisov21
Copy link
Contributor

+1 big idea!

@ficristo
Copy link
Collaborator

This needs more work. Closing for now.

@ficristo ficristo closed this Feb 13, 2017
@petetnt petetnt reopened this Mar 1, 2017
@petetnt
Copy link
Collaborator

petetnt commented Mar 1, 2017

LGTM sans tests, merging this and will open another issue for the tests as per #11763 (comment)

Thanks for this great contribution @mansimarkaur and sorry for taking so long!

PS: did this test totally flake? https://travis-ci.org/adobe/brackets/builds/206719157 CLA checked manually just in case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants