-
Notifications
You must be signed in to change notification settings - Fork 364
Conversation
It would be nice to have this as a checkbox in the settings view which means it would need a true/false value and clear name that articulated the default value So how about naming it |
A nit, but I'd rather it were called |
@benogle good point, let's go with |
No problems, will fix it asap! |
@kevinsawicki There's a point unclear to me. Edit: I kept the menu items unchanged since there is no way to add checkmarks to them, and also because I noticed you are using |
Also added default configuration value
@VictorGama yes, you can use |
@kevinsawicki Perfect! |
Yeah, that works |
|
||
toggleSide: -> | ||
newValue = !atom.config.get('tree-view.showOnRightSide') | ||
atom.config.set('tree-view.showOnRightSide', newValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an atom.config.toggle
method that comes in handy here.
atom.config.toggle('tree-view.showOnRightSide')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 09e1526
@@ -150,7 +156,9 @@ class TreeView extends ScrollView | |||
$(document.body).off('mouseup', @resizeStopped) | |||
|
|||
resizeTreeView: ({pageX}) => | |||
@width(pageX) | |||
w = pageX | |||
w = $('body').width() - w if atom.config.get('tree.toggleSide') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be tree-view.showOnRightSide
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops! Sorry.
Edit: Whoa. I forgot several rules. Thanks for the heads up!
onSideToggled: (newValue) -> | ||
@detach() | ||
@attach() | ||
$(this).attr('data-toggle-showOnRightSide', newValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use @attr
here instead of $(this).attr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9e11874
Also removed console.log call
@@ -16,7 +16,7 @@ FileView = require './file-view' | |||
module.exports = | |||
class TreeView extends ScrollView | |||
@content: -> | |||
@div class: 'tree-view-resizer tool-panel panel-left', => | |||
@div class: 'tree-view-resizer tool-panel', 'data-showOnRightSide': atom.config.get('tree-view.showOnRightSide'), => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really minor feedback but I think this data attribute would read better if it was dasherized instead of camel cased since it is referenced in the .less file and class names are dasherized there.
So data-show-on-right-side
instead of data-showOnRightSide
@kevinsawicki Dasherized :) |
@@ -16,7 +16,7 @@ FileView = require './file-view' | |||
module.exports = | |||
class TreeView extends ScrollView | |||
@content: -> | |||
@div class: 'tree-view-resizer tool-panel panel-left', => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A .panel-right
style needs to be added to the default light/dark ui themes and then I think this is ready to be merged.
Since this is the first thing going on the right side there are no existing styles and not having .panel-left
on it removes the border which is visually important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I add these classes on @attach
?
Insert a ternary operator in the middle of this structure will be pretty bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, for sure, @attach
is the right place to handle them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed on 358cb13
I think this is almost ready, just need to get some I've opened up issues on the appropriate repos. |
Looks like some code leaked into the wrong branch. My fault. |
Add 'Toggle Tree Side' menu item
Thanks for all your hard work on this, I've merged it into master. 👍 |
I'm currently on I'm all for hacking config files, but for something as simple as a true/false, when the above 'Show...' have a similar true/false flag alread in the settings, then that's where I expected it. edit: Found the menu under Package -> Tree View -> Toggle Tree Side |
Thank you so much for this! #CodeOnTheLeft4Life |
This adds a new menu item called 'Toggle Tree Side', that toggles the tree view side between right and left