-
Notifications
You must be signed in to change notification settings - Fork 1
[WIP] Configuration View #19
base: master
Are you sure you want to change the base?
Changes from 5 commits
9cc5f06
1f61d48
a097af5
ead72c5
5826601
6fb0b90
b101805
5188eac
3e28be4
e01ab49
ab6b8a7
6e431ac
833fe7a
9d6f6a4
76087a8
a82d104
01b483b
6477f18
9135187
8ef925b
27f8cff
fc52617
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 |
---|---|---|
@@ -0,0 +1,48 @@ | ||
.configuration { | ||
flex-grow: 1; | ||
flex-shrink: 1; | ||
display: flex; | ||
flex-direction: column; | ||
font-weight: normal; | ||
} | ||
|
||
.configuration__header { | ||
height: 61px; | ||
border-bottom: 1px solid #DDD; | ||
display: flex; | ||
align-items: center; | ||
padding: 8px; | ||
flex-shrink: 0; | ||
} | ||
|
||
.configuration__headerTitle { | ||
margin: 0; | ||
padding: 0; | ||
font-size: 18px; | ||
} | ||
|
||
.configuration__body { | ||
flex-grow: 1; | ||
display: flex; | ||
} | ||
|
||
.configuration__sidebar { | ||
width: 256px; | ||
border-right: 1px solid #DDD; | ||
} | ||
|
||
.configuration__sidebarHeader { | ||
background-color: #EEE; | ||
padding: 4px 8px; | ||
border-bottom: 1px solid #DDD; | ||
font-weight: bold; | ||
} | ||
|
||
.configuration__content { | ||
flex-grow: 1; | ||
} | ||
|
||
.configuration__contentPlaceholder { | ||
padding-top: 32px; | ||
text-align: center; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
.treenode { | ||
display: flex; | ||
flex-direction: column; | ||
} | ||
|
||
.treenode__header { | ||
display: flex; | ||
align-items: center; | ||
height: 28px; | ||
padding: 4px 8px; | ||
cursor: pointer; | ||
user-select: none; | ||
} | ||
|
||
.treenode__header:hover { | ||
background-color: #EEE; | ||
color: #1976D2; | ||
} | ||
|
||
.treenode__icon { | ||
margin-right: 4px; | ||
} | ||
|
||
.treenode__icon svg { | ||
display: block; | ||
width: 18px; | ||
height: 18px; | ||
} | ||
|
||
.treenode__label { | ||
flex-grow: 1; | ||
} | ||
|
||
.treenode__children { | ||
display: none; | ||
} | ||
|
||
.treenode--expanded > .treenode__children { | ||
display: block; | ||
} | ||
|
||
.treenode--selected > .treenode__header { | ||
color: white; | ||
background-color: #2196F3; | ||
} | ||
|
||
.treenode--selected > .treenode__header svg { | ||
fill: rgba(255, 255, 255, 0.75); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
import * as classnames from 'classnames' | ||
import { observer } from 'mobx-react' | ||
import * as React from 'react' | ||
|
||
import FileIcon from './file.svg' | ||
import FolderIconOpen from './folder-open.svg' | ||
import FolderIcon from './folder.svg' | ||
import * as style from './style.css' | ||
|
||
export interface Node { | ||
label: string | ||
expanded: boolean | ||
leaf: boolean | ||
selected: boolean | ||
children?: Node[] | ||
} | ||
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. You can remove all the commas from these interface definitions, e.g.
I'd put it in a tslint rule if there was one (palantir/tslint#960)... 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. Gotcha. |
||
|
||
export interface TreeProps { | ||
data: Node | ||
level?: number | ||
onClick(node: Node): void | ||
} | ||
|
||
@observer | ||
export class Tree extends React.Component<TreeProps, any> { | ||
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. The second param type 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. Fixed. |
||
constructor(props: any, context: any) { | ||
super(props, context) | ||
this.onClick = this.onClick.bind(this) | ||
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. This line is not needed when you use class properties (which you're already doing!), so you can delete this line 🙂 Normal class method, needs the
Arrow function class property, automatically binds for you:
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. Nice, didn't know that! |
||
} | ||
|
||
public render(): JSX.Element { | ||
const children = this.props.data.children || [] | ||
const hasChildren = children.length > 0 | ||
const level = this.props.level || 0 | ||
const classes = classnames( | ||
style.treenode, | ||
{ [style['treenode--expanded']]: this.props.data.expanded }, | ||
{ [style['treenode--selected']]: this.props.data.selected }, | ||
) | ||
const headerInlineStyle = { paddingLeft: 8 + (level * 22) + 'px' } | ||
|
||
return ( | ||
<div className={classes}> | ||
<div className={style.treenode__header} onClick={this.onClick} style={headerInlineStyle}> | ||
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. Could we use e.g. http://jsbin.com/zoyabovami/edit?html,css,output Credit to @MonicaOlejniczak for the idea 🙂 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. 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. Hmmm, good point! You can sorta do it with pure CSS but it's pretty hacky. 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. Ah! Makes sense. Maybe add a comment explaining why then? 🙂 Otherwise the next person will think the same thing haha 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. We can still use 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. Done: 6e431ac |
||
<div className={style.treenode__icon}> | ||
{ | ||
hasChildren ? | ||
(this.props.data.expanded ? <FolderIconOpen /> : <FolderIcon />) : | ||
<FileIcon /> | ||
} | ||
</div> | ||
|
||
<div className={style.treenode__label}>{ this.props.data.label }</div> | ||
</div> | ||
|
||
<div className={style.treenode__children}> | ||
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. Can we optimise this by only rendering children if it is expanded? Then we can delete the higher specificity CSS selectors like 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. Maybe something like:
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. Done! |
||
{ children.map((child, i) => <Tree key={i} data={child} level={level + 1} onClick={this.props.onClick} />) } | ||
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. Are we safe to use Source: https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318 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. Wasn't sure what to use as the key, and React was complaining without it, that's why I used the index. We could use |
||
</div> | ||
</div> | ||
) | ||
} | ||
|
||
private onClick = (e: any) => { | ||
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. You can use 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. Using ERROR in [at-loader] ./src/client/components/configuration/tree/tree.tsx:44:14
TS2322: Type '{ className: any; onClick: (e: MouseEvent) => void; style: { paddingLeft: string; }; }' is not assignable to type 'HTMLProps<HTMLDivElement>'.
Types of property 'onClick' are incompatible.
Type '(e: MouseEvent) => void' is not assignable to type 'EventHandler<MouseEvent<HTMLDivElement>> | undefined'.
Type '(e: MouseEvent) => void' is not assignable to type 'EventHandler<MouseEvent<HTMLDivElement>>'.
Types of parameters 'e' and 'event' are incompatible.
Type 'MouseEvent<HTMLDivElement>' is not assignable to type 'MouseEvent'.
Property 'fromElement' is missing in type 'MouseEvent<HTMLDivElement>'. |
||
this.props.onClick(this.props.data) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import { action, observable } from 'mobx' | ||
import { Node } from './tree/tree' | ||
|
||
export class TreeStore { | ||
@observable | ||
public data: Node | ||
|
||
@observable | ||
public selectedNode: Node | ||
|
||
constructor(data: Node) { | ||
this.data = data | ||
this.onNodeClick = this.onNodeClick.bind(this) | ||
} | ||
|
||
@action | ||
public onNodeClick(node: Node): void { | ||
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. One of my architectural goals is to keep models as dumb as possible, such that they are just data and no behaviour. That means actions like these need to live somewhere else, preferably somewhere that we can test. This is why I created a Could we follow a similar pattern here? 😄 You'll probably want a separate 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. Sounds good, will do. |
||
if (node.leaf) { | ||
this.selectNode(node) | ||
} else { | ||
this.toggleExpansion(node) | ||
} | ||
} | ||
|
||
@action | ||
public toggleExpansion(node: Node) { | ||
node.expanded = !node.expanded | ||
} | ||
|
||
@action | ||
public selectNode(node: Node) { | ||
if (this.selectedNode) { | ||
this.selectedNode.selected = false | ||
} | ||
|
||
node.selected = true | ||
this.selectedNode = node | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import * as React from 'react' | ||
import * as style from './style.css' | ||
import { Node, Tree } from './tree/tree' | ||
import { TreeStore } from './tree_store' | ||
|
||
const store = new TreeStore({ | ||
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. I think its best that we stop using the word 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. Done! |
||
label: 'root', | ||
expanded: true, | ||
leaf: false, | ||
selected: false, | ||
children: [ | ||
{ | ||
label: 'parent', | ||
expanded: false, | ||
leaf: false, | ||
selected: false, | ||
children: [ | ||
{ label: 'child1', expanded: false, leaf: true, selected: false }, | ||
{ label: 'child2', expanded: false, leaf: true, selected: false }, | ||
], | ||
}, | ||
{ | ||
label: 'parent', | ||
expanded: false, | ||
leaf: false, | ||
selected: false, | ||
children: [ | ||
{ | ||
label: 'nested parent', | ||
expanded: false, | ||
leaf: false, | ||
selected: false, | ||
children: [ | ||
{ label: 'nested child 1', expanded: false, leaf: true, selected: false }, | ||
{ label: 'nested child 2', expanded: false, leaf: true, selected: false }, | ||
], | ||
}, | ||
], | ||
}, | ||
{ | ||
label: 'Some file here', | ||
expanded: true, | ||
leaf: true, | ||
selected: false, | ||
children: [] as Node[], | ||
}, | ||
], | ||
}) | ||
|
||
export const Configuration = () => ( | ||
<div className={style.configuration}> | ||
<div className={style.configuration__header}> | ||
<h1 className={style.configuration__headerTitle}>Configuration</h1> | ||
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. We'll have to make a component for this header stuff 🙂 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. I've made two wrapper components: view (for top-level views like dashboard or localization) and panel (for stuff like sidebars) which could be useful. I placed them under I can extract them into a separate pull request if needed. |
||
</div> | ||
|
||
<div className={style.configuration__body}> | ||
<div className={style.configuration__sidebar}> | ||
<div className={style.configuration__sidebarHeader}>Files</div> | ||
<Tree onClick={store.onNodeClick} data={store.data} /> | ||
</div> | ||
|
||
<div className={style.configuration__content}> | ||
<div className={style.configuration__contentPlaceholder}> | ||
Select a file to edit | ||
</div> | ||
</div> | ||
</div> | ||
</div> | ||
) |
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.
Do you think this will ever come back to bite us? Would libraries ever assume
content-box
is the default and break? Maybe its fine and we'll revisit if its a problem.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.
Another option is this, which allows for easily resetting the
box-sizing
locally, when needed:Reference: https://css-tricks.com/box-sizing/#article-header-id-6