-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Stop using flexbox in editor's parent hierarchy #1847
Changes from 3 commits
3c2aa36
e258103
2caf138
8a3bac9
addb846
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,10 +94,14 @@ define(function (require, exports, module) { | |
$sidebar.width(width); | ||
$sidebarResizer.css("left", width - 1); | ||
|
||
// the following three lines help resize things when the sidebar shows | ||
// but ultimately these should go into ProjectManager.js with a "notify" | ||
// event that we can just call from anywhere instead of hard-coding it. | ||
// waiting on a ProjectManager refactor to add that. | ||
// This maintains the editor position to the right of the sidebar. (A simple float is not enough | ||
// because the non-floated content technically underlaps the floated sidebar; this breaks CodeMirror | ||
// listeners and relative positioning). | ||
// TODO: Ultimately this should go into EditorManager.js, triggered via an event we dispatch | ||
$(".content", $sidebar.parent()).css("margin-left", width); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reminder: Also need to do this in toggleSidebar(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My fix for that bug you found is a little different: doing this in the isSidebarClosed case of _setWidth() rather than just the else will fix it without needing to modify toggleSidebar() (since it calls _setWidth()). |
||
|
||
// This helps resize things within the sidebar when it's shown. | ||
// TODO: Ultimately this should go into ProjectManager.js, triggered via an event we dispatch | ||
$sidebar.find(".sidebar-selection").width(width); | ||
|
||
if (width > 10) { | ||
|
@@ -146,6 +150,7 @@ define(function (require, exports, module) { | |
|
||
$sidebarResizer.css("left", sidebarWidth - 1); | ||
|
||
// Initially size sidebar at app startup | ||
if (prefs.getValue("sidebarClosed")) { | ||
toggleSidebar(sidebarWidth); | ||
} else { | ||
|
@@ -169,7 +174,7 @@ define(function (require, exports, module) { | |
|
||
isMouseDown = true; | ||
|
||
// take away the shadows (for performance reasons during sidebarmovement) | ||
// take away the shadows (for performance reasons during sidebar movement) | ||
$sidebar.find(".scroller-shadow").css("display", "none"); | ||
|
||
$body.toggleClass("resizing"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ html, body { | |
} | ||
|
||
body { | ||
.vbox; | ||
height: 100%; | ||
|
||
&.resizing a, &.resizing #projects a, &.resizing .main-view, &.resizing .CodeMirror-lines { | ||
cursor: col-resize; | ||
|
@@ -88,17 +88,17 @@ a, img { | |
} | ||
|
||
.main-view { | ||
.hbox; | ||
.box-flex(1); | ||
height: 100%; | ||
|
||
.sidebar { | ||
height: 100%; | ||
.vbox; | ||
width: @sidebar-width; | ||
float: left; | ||
} | ||
|
||
.content { | ||
.vbox; | ||
.box-flex(1); | ||
height: 100%; | ||
} | ||
} | ||
|
||
|
@@ -222,13 +222,11 @@ a, img { | |
} | ||
|
||
#editor-holder { | ||
.vbox; | ||
.box-flex(1); | ||
position: relative; | ||
|
||
/* Placeholder shown when there is no editor open */ | ||
#not-editor { | ||
.box-flex(1); | ||
height: 100%; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yay! Good riddance flex-box! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, well, if it performed well we would love to use it. Floats and margins make me nauseous. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I totally agree. Maybe one of these days we can donate to webkit the faster implementation Flex used for VBox/HBox ;-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory, flex-box sounds awesome, but it took very little code to replace it. Yeah, I've been using floats and margins for years and they still seem wrong :) Maybe the sidebar/editor layout could have been implemented using inline-boxes, but we may have run into the same performance problems as with flex-box. |
||
.vbox; | ||
.box-pack(center); | ||
.box-align(center); | ||
|
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 code implies that all children of editor-holder are containers which are stacked vertically. Currently they are, but someone could add a new element that doesn't play by that rule. At the very least, you should state this in a comment in the main-view.html file. You could also verify this assumption programmatically, but that may impact performance.
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.
Good point. I'll add a comment in main-view.html.