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

Toggle panels and no-Distraction mode #11732

Merged
merged 12 commits into from Nov 18, 2015
Merged

Toggle panels and no-Distraction mode #11732

merged 12 commits into from Nov 18, 2015

Conversation

abose
Copy link
Contributor

@abose abose commented Sep 23, 2015

Full code view with all other panels/toolbars hidden.
Also minor refactoring of variable names for toggle/hide for sidebar.
shortcut : ctrl+shift+` for toggle panels
shortcut : ctrl+shift+2 for no distraction mode
Also added panel management API's to workspace manager.

@nethip

  • Add unit tests

@abose abose added this to the Release 1.6 milestone Sep 23, 2015
@abose abose changed the title no-Distraction mode Toggle panels and no-Distraction mode Oct 7, 2015
@abose
Copy link
Contributor Author

abose commented Oct 8, 2015

Should it be hide panels or toggle panels? toggle seems to be troublesome at times.

@abose
Copy link
Contributor Author

abose commented Oct 12, 2015

@petetnt @ficristo @zaggino

/**
* hide all open panels
*/
function _hidePanlesIfRequired() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small typo: Panles -> `Panels"

@petetnt
Copy link
Collaborator

petetnt commented Oct 12, 2015

I think that hidePanels should be sufficient, at least until the panels are grouped together with something like #11586. Panels get currently opened and closed all the time so it would be tons of trouble to keep up with their states at this point IMO.

Made a few comments on the code, just some small typos.

The strings use "No distractions" and "Code view only" as the names, should those be unified to one of them?

CMD_TOGGLE_PURE_CODE = "view.togglePureCode",
CMD_TOGGLE_PANELS = "view.togglePanels";

//key biding keys
Copy link
Collaborator

Choose a reason for hiding this comment

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

binding

@abose
Copy link
Contributor Author

abose commented Oct 13, 2015

Thanks @petetnt for reviewing.
Made the change to toggle panel only if workspace has not changed after a hide panels command is issued. This looked intuitive to me.
The documentation for the preference was intentionally worded as Code view only. The string describes what no distractions mode in simple terms.

@@ -61,12 +62,13 @@ define(function (require, exports, module) {
// Load dependent modules
var AppInit = require("utils/AppInit"),
EventDispatcher = require("utils/EventDispatcher"),
viewUtils = require("utils/ViewUtils"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: ViewUtils, in ProperCase

@ficristo
Copy link
Collaborator

In the View menu, Toggle Panels menu doesn't show the shortcut.

@abose I briefly tested the patch and seems fine.
I agree with @petetnt for the panels.

@abose
Copy link
Contributor Author

abose commented Oct 26, 2015

@zaggino There seems to be a bug. If you hide the git panel, the blue active icon for the git extension still remains in the main-toolbar. So added events when panels are shown or hidden so that extensions can listen to panel close events.

@zaggino
Copy link
Contributor

zaggino commented Oct 26, 2015

thanks for the info @abose

@petetnt
Copy link
Collaborator

petetnt commented Oct 27, 2015

Tested this out again against 51c0232 , love the No distractions mode.

There's a small issue with toggle panels though (other than the shortcut not showing up/working)

  1. Open foo.js
  2. Create a validation error (I am using brackets-eslint)
  3. Click Toggle Panels -> nothing happens (expected: panel closes)
  4. Click Toggle Panels -> panel closes.

That issue that affects brackets-git affects other Extensions too, such as brackets-todo. Is there a way to (at least temporarily / during 1.6.) to throw a warning if panel was closed but the button state remained + link to say Brackets-Wiki where those events would be explained). That would provide a easy way for extension makers that use panels to know that they should update their extensions).

@ryanstewart
Copy link
Contributor

Looks good to me. And agree, that the no distraction mode is a nice touch.

Do we have an idea of how often people will want to jump into this? Main question is wether we can set an easier shortcut.

I also like that it doesn't go full screen the way Sublime Text does, but any thought about whether we need that?

@petetnt
Copy link
Collaborator

petetnt commented Nov 10, 2015

@ryanstewart there was an issue about "true" fullscreen some while ago (#11594) and when checking out that one I saw that CEF got updated to support fullscreen quite recently so it could be implemented relatively easily (needs a brackets-shell update though) separate from this feature. 👍

@ryanstewart
Copy link
Contributor

@petetnt ahh, awesome. And I'm definitely not advocating for that, just wanted to throw it out there. Sounds like it's been covered though!

@abose
Copy link
Contributor Author

abose commented Nov 17, 2015

ctrl+shift+~ falls in the same vertical last line in the keyboard, so i thought it would be easily accesible to dismiss panels with one hand.

@abose
Copy link
Contributor Author

abose commented Nov 17, 2015

@petetnt i have created #11924
This is an issue in extract extension too.

@@ -0,0 +1,155 @@
/*
* Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Change copyright from 2013 to 2015

@nethip
Copy link
Contributor

nethip commented Nov 18, 2015

Couple of quick comments.

  • We need to add unit tests for the new No Distraction feature. I saw some changes in the unit tests but I am not sure if they are going to cover the new workflows
  • We need to add health logs for the new functionality. Also make sure to update our Health Data wiki with this new data
  • It is better to have null checks at places where we calling APIs, as we would never know if the API returns an actual value or null. If you want to catch errors here, use a null check along with console.warn/console.error. That should raise errors in the console, in case of failures
  • Please update the documentation, wherever required
  • Make sure we update our API docs with the changes/additions

Overall it looks good 👍 It is good to merge, provided the review comments are addressed.

@abose
Copy link
Contributor Author

abose commented Nov 18, 2015

@nethip thanks for reviewing. Made the changes.
Will add unit tests soon.

@nethip
Copy link
Contributor

nethip commented Nov 18, 2015

@abose No problem :) I took the liberty to add unit tests checkbox to your first comment. Also I created a waffle card for tracking the unit tests for this feature I am going ahead merging this.

Thanks for this feature @abose !

nethip added a commit that referenced this pull request Nov 18, 2015
Toggle panels and no-Distraction mode
@nethip nethip merged commit 3cec328 into master Nov 18, 2015
@marcelgerber marcelgerber deleted the abose/noDistractions branch May 23, 2016 07:58
@seehup
Copy link

seehup commented Oct 5, 2018

Comment on pull request

1 similar comment
@seehup
Copy link

seehup commented Oct 5, 2018

Comment on pull request

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

7 participants