-
Notifications
You must be signed in to change notification settings - Fork 146
Conversation
@alexkuz it looks like CI is failing with these changes. Would you mind adding/fixing tests for this functionality? |
@@ -8,7 +8,7 @@ var EventLoopView = require("./views/eventloop-view"); | |||
var MemoryView = require("./views/memory-view"); | |||
var CpuView = require("./views/cpu-view"); | |||
var HelpView = require("./views/help"); | |||
var layouts = require("./layouts"); | |||
var generateLayouts = require("./generate-layouts"); |
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 file doesn't seem to exist?
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.
Oops! Missed it.
I'll fix tests, you can try it out meanwhile (with Forgot to mention - I also removed |
@jasonwilson tests are ready With a bit more of refactoring, it would be easy to implement {
name: "stdout",
type: "log",
streams: ["stdout"],
exclude: "^\[STATUS\]"
},
{
name: "stderr",
type: "log",
streams: ["stderr"],
exclude: "^\[STATUS\]"
},
{
position: {
size: 3
},
name: "status",
type: "log",
streams: ["stdout", "stderr"],
include: "^\[STATUS\](.*)"
} |
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.
Overall looks good- just want to convert layouts to js
and pull default into its own file.
lib/generate-layouts.js
Outdated
// View or panel with 'grow' fills <grow> part of the residuary space (it works like flex-grow). | ||
// By default, position = { grow: 1 } | ||
|
||
var DEFAULT_LAYOUT_CONFIG = [ |
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.
The default config should live in a separate file, say ./layouts/default.js
. Do you see any problem with using js instead of json?
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 are right, js is better here. I replaced fs
with require
- not completely sure if I require there in a right way, but it seems to work.
Added layout config validation with |
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 to me
Phew!
So, I refactored layouts a little bit. Now user can provide JSON with layout configurations, which is then converted to internal layout object. Layout configuration describes panels (or columns), each contains views. Each panel and view has
position
object, that have asize
orgrow
property.size
defines exact width for panels and height for views, andgrow
works like CSSflex-grow
. So panel with{ grow: 2 }
will be twice as wide as panel with{ grow: 1 }
.There are also some fixes to prevent crashes,
view.layoutConfig
is now optional (view is hidden if there is no config).