Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

[WIP] Configuration View #19

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9cc5f06
Initial work on Configuration
JosephusPaye May 18, 2017
1f61d48
Convert indents to 2-spaces
JosephusPaye May 19, 2017
a097af5
Add trailing newline to SVG files, use 2 space indents
JosephusPaye May 19, 2017
ead72c5
Remove trailing commas from interfaces
JosephusPaye May 19, 2017
5826601
Remove default exports
JosephusPaye May 19, 2017
6fb0b90
Merge branch 'master' into feature/configuration
JosephusPaye May 22, 2017
b101805
Remove unnecessary tree constructor, render children only when expanded
JosephusPaye May 24, 2017
5188eac
Rename tree_store to tree_model
JosephusPaye May 24, 2017
3e28be4
Extract sidebar wrapper to standalone panel component
JosephusPaye May 24, 2017
e01ab49
Extract view wrapper (and header) to dedicated view component
JosephusPaye May 24, 2017
ab6b8a7
Add controller and model, move sample data to separate file
JosephusPaye May 24, 2017
6e431ac
Use ul/li tags for tree
JosephusPaye May 24, 2017
833fe7a
Add comment about inline paddingLeft
JosephusPaye May 24, 2017
9d6f6a4
Tweak indentation comment
JosephusPaye May 24, 2017
76087a8
Merge origin/master
JosephusPaye Jun 20, 2017
a82d104
Merge branch 'master' into feature/configuration
JosephusPaye Jun 22, 2017
01b483b
Merge branch 'master' into feature/configuration
JosephusPaye Jun 23, 2017
6477f18
Merge branch 'master' into feature/configuration
JosephusPaye Jun 27, 2017
9135187
WIP configuration editor
JosephusPaye Jun 27, 2017
8ef925b
Make the basic editor work, fix styling
JosephusPaye Jun 30, 2017
27f8cff
Merge branch 'master' into feature/configuration
JosephusPaye Jul 14, 2017
fc52617
.
JosephusPaye Jul 26, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/client/components/app/style.css
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
@import url('https://fonts.googleapis.com/css?family=Open+Sans:300,400,500');

* {
box-sizing: border-box;
Copy link
Member

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.

Copy link
Member Author

@JosephusPaye JosephusPaye May 19, 2017

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:

html {
  box-sizing: border-box;
}

*, *:before, *:after {
  box-sizing: inherit;
}

Reference: https://css-tricks.com/box-sizing/#article-header-id-6

}

html,
body {
margin: 0;
Expand Down
48 changes: 48 additions & 0 deletions src/client/components/configuration/style.css
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;
}
3 changes: 3 additions & 0 deletions src/client/components/configuration/tree/file.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions src/client/components/configuration/tree/folder-open.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions src/client/components/configuration/tree/folder.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
49 changes: 49 additions & 0 deletions src/client/components/configuration/tree/style.css
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);
}
66 changes: 66 additions & 0 deletions src/client/components/configuration/tree/tree.tsx
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[]
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove all the commas from these interface definitions, e.g.

export interface Node {
  label: string
  expanded: boolean
  leaf: boolean
  selected: boolean
  children?: Node[]
}

I'd put it in a tslint rule if there was one (palantir/tslint#960)...

Copy link
Member Author

Choose a reason for hiding this comment

The 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second param type any should be void I believe

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Member

@BrendanAnnable BrendanAnnable May 19, 2017

Choose a reason for hiding this comment

The 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 .bind in constructor if you want to pass it directly to something which won't keep the context:

onClick(e: MouseEvent) {
  // "this" might change
}

Arrow function class property, automatically binds for you:

onClick = (e: MouseEvent) => {
  // "this" will always be the class instance
}

Copy link
Member Author

Choose a reason for hiding this comment

The 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}>
Copy link
Member

@BrendanAnnable BrendanAnnable May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use ul and li for this HTML structure so we don't need to use a calculated paddingLeft?

e.g. http://jsbin.com/zoyabovami/edit?html,css,output

Credit to @MonicaOlejniczak for the idea 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that initially (see below) but changed to paddingLeft because I wanted the hover and selected highlights to be full width. I can switch back if necessary.

Using ul/li:

Using div and paddingLeft:

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

@BrendanAnnable BrendanAnnable May 24, 2017

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still use ul and li though at least for the semantics

Copy link
Member Author

Choose a reason for hiding this comment

The 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}>
Copy link
Member

Choose a reason for hiding this comment

The 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 .treenode--expanded > .treenode__children

Copy link
Member

@BrendanAnnable BrendanAnnable May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like:

{this.props.data.expanded &&
  <div className={style.treenode__children}>
  ...
  </div>
}

Copy link
Member Author

Choose a reason for hiding this comment

The 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} />) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@JosephusPaye JosephusPaye May 24, 2017

Choose a reason for hiding this comment

The 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 label + level, but there could be duplicates.

</div>
</div>
)
}

private onClick = (e: any) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use MouseEvent instead of any here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using MouseEvent gives this error:

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)
}
}
39 changes: 39 additions & 0 deletions src/client/components/configuration/tree_store.tsx
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 {
Copy link
Member

Choose a reason for hiding this comment

The 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 LocalisationPresenter for the localisation component, and any @action lives as a method on the presenter, such that the LocalisationModel itself does not have any actions on it. I'll soon be renaming LocalisationPresenter to LocalisationController as well.

Could we follow a similar pattern here? 😄 You'll probably want a separate ConfigurationController class where these actions can live 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The 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
}
}
69 changes: 69 additions & 0 deletions src/client/components/configuration/view.tsx
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({
Copy link
Member

@BrendanAnnable BrendanAnnable May 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its best that we stop using the word store and model and just use one throughout the code base (or at least come to a definition of the difference). Then we can maybe think of it as a more traditional "model view controller" architecture. So I suggest we replace every occurrence of store with model.

Copy link
Member Author

Choose a reason for hiding this comment

The 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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to make a component for this header stuff 🙂

Copy link
Member Author

@JosephusPaye JosephusPaye May 24, 2017

Choose a reason for hiding this comment

The 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 components/configuration for now, but I think they should go higher (components/view|panel/ or components/common/view|panel/?) since they could be used by other views.

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>
)
1 change: 1 addition & 0 deletions src/client/components/navigation/view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const NavigationView = () => (
<NavigationItemView url='/classifier' Icon={CubeIcon}>Classifier</NavigationItemView>
<NavigationItemView url='/subsumption' Icon={OrderingIcon}>Subsumption</NavigationItemView>
<NavigationItemView url='/gamestate' Icon={ControllerIcon}>GameState</NavigationItemView>
<NavigationItemView url='/configuration' Icon={ControllerIcon}>Configuration</NavigationItemView>
</ul>
</header>
)
Expand Down
2 changes: 2 additions & 0 deletions src/client/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import * as io from 'socket.io-client'
import { AppView } from './components/app/view'
import { Chart } from './components/chart/view'
import { Classifier } from './components/classifier/view'
import { Configuration } from './components/configuration/view'
import { Dashboard } from './components/dashboard/view'
import { GameState } from './components/game_state/view'
import { RobotModel } from './components/localisation/darwin_robot/model'
Expand Down Expand Up @@ -96,6 +97,7 @@ ReactDOM.render(
<Route path='/classifier' component={Classifier}/>
<Route path='/subsumption' component={Subsumption}/>
<Route path='/gamestate' component={GameState}/>
<Route path='/configuration' component={Configuration}/>
</Route>
</Router>
</Provider >,
Expand Down