-
Notifications
You must be signed in to change notification settings - Fork 49
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
Feature/sidepanel resize #225
Conversation
var $content = $('.main'); | ||
var $sidepanel = $('.aside'); | ||
|
||
var isSidepanelResized = false; |
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.
Move consts outside link function
|
||
function linkFunc($scope, $element) { | ||
var $body = $('body'); | ||
var $content = $('.main'); |
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.
Is there a better way of getting that elements? Can't the directive be attached to higher element in DOM and use elements in it's scope?
@@ -22,6 +22,8 @@ define(['angularAMD'], function (angularAMD) { | |||
['$rootScope', 'localStorageService', ToggleSidepanelDirective]); | |||
|
|||
function ToggleSidepanelDirective($rootScope, localStorageService) { | |||
var $content = $('.main'); |
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.
This is also duplicated in the second directive, there should be a way of making that in single place
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.
Looks good 👏
$sidepanel.css('left', 0); | ||
|
||
$(window).on('resize', onWindowResize); |
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.
Add throttle here
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.
Done
localStorageService.put(EXPANDED_SIDEBAR_KEY_NAME, true); | ||
} | ||
|
||
function close() { | ||
$content.css('left', 0); | ||
$content.css('width', $body.width()); | ||
$content.css('width', '100%'); |
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.
Can't we just remove width then?
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.
Yup, removed width inline style 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.
Looks good :)
Description
Added possibility of resizing sidepanel and updated sidepanel toggle functionality to work correctly with different widths of sidepanel
Motivation and Context
Test names can get pretty long and are hard to read with current width of the sidepanel.
Screenshots (if appropriate):
Types of changes
Checklist:
I hereby agree to the terms of the AET Contributor License Agreement.