[WIP] Configuration View #19

JosephusPaye · 2017-05-18T12:29:10Z

Adding the configuration view. Feedback welcome.

- Includes a basic working Tree component for browsing configuration files

BrendanAnnable

Quick review on my way to work, I didn't get a chance to run the code yet but just some high level feedback 🙂

BrendanAnnable · 2017-05-18T23:38:58Z

src/client/components/configuration/style.css

+	flex-shrink: 1;
+	display: flex;
+	flex-direction: column;
+    font-weight: normal;


Could you reformat all these CSS files to use 2 space indentation? 🙂 I think its my fault for checking in some non-2-space files.

I should probably should look into adding something like https://github.com/stylelint/stylelint

BrendanAnnable · 2017-05-18T23:40:12Z

src/client/components/configuration/tree/tree.tsx

+  leaf: boolean,
+  selected: boolean,
+  children?: Node[]
+}


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

BrendanAnnable · 2017-05-18T23:41:27Z

src/client/components/configuration/view.tsx

+  </div>
+)
+
+export default Configuration


I'm going to be removing all default exports, so might as well remove this one 😄

BrendanAnnable · 2017-05-18T23:45:15Z

src/client/components/configuration/tree_store.tsx

+  }
+
+  @action
+  public onNodeClick(node: Node): void {


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 🙂

Sounds good, will do.

BrendanAnnable · 2017-05-18T23:52:10Z

src/client/components/app/style.css

@@ -1,5 +1,9 @@
 @import url('https://fonts.googleapis.com/css?family=Open+Sans:300,400,500');

+* {
+	box-sizing: border-box;


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.

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

BrendanAnnable · 2017-05-18T23:53:02Z

src/client/components/configuration/tree/file.svg

@@ -0,0 +1,3 @@
+<svg width="16" height="16" fill="#999" viewBox="0 0 24 24">
+    <path d="M13,9H18.5L13,3.5V9M6,2H14L20,8V20A2,2 0 0,1 18,22H6C4.89,22 4,21.1 4,20V4C4,2.89 4.89,2 6,2M11,4H6V20H11L18,20V11H11V4Z" />
+</svg>


Can you ensure all these SVG files have a trailing newline? 🙂

BrendanAnnable · 2017-05-18T23:53:12Z

src/client/components/configuration/tree/tree.tsx

+  }
+}
+
+export default Tree


I'm going to be removing all default exports, so might as well remove this one 😄

JosephusPaye · 2017-05-19T01:24:33Z

@BrendanAnnable Thanks for the suggestions, I'll make the requested changes and push updates soon.

BrendanAnnable

Got a chance to run it, lookin' fancy! 😄

BrendanAnnable · 2017-05-19T08:02:16Z

src/client/components/configuration/tree/tree.tsx

+        </div>
+
+        <div className={style.treenode__children}>
+          { children.map((child, i) => <Tree key={i} data={child} level={level + 1} onClick={this.props.onClick} />) }


Are we safe to use i as a key here?

Source: https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318

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.

BrendanAnnable · 2017-05-19T08:28:54Z

src/client/components/configuration/tree/tree.tsx

+          <div className={style.treenode__label}>{ this.props.data.label }</div>
+        </div>
+
+        <div className={style.treenode__children}>


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

Maybe something like:

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

BrendanAnnable · 2017-05-19T08:29:23Z

src/client/components/configuration/tree/tree.tsx

+    )
+  }
+
+  private onClick = (e: any) => {


You can use MouseEvent instead of any here

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

BrendanAnnable · 2017-05-19T08:32:06Z

src/client/components/configuration/view.tsx

+import { Node, Tree } from './tree/tree'
+import { TreeStore } from './tree_store'
+
+const store = new TreeStore({


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.

BrendanAnnable · 2017-05-19T08:33:32Z

src/client/components/configuration/view.tsx

+export const Configuration = () => (
+  <div className={style.configuration}>
+    <div className={style.configuration__header}>
+      <h1 className={style.configuration__headerTitle}>Configuration</h1>


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

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.

BrendanAnnable · 2017-05-19T08:33:47Z

src/client/components/configuration/tree/tree.tsx

+}
+
+@observer
+export class Tree extends React.Component<TreeProps, any> {


The second param type any should be void I believe

BrendanAnnable · 2017-05-19T08:44:11Z

src/client/components/configuration/tree/tree.tsx

+export class Tree extends React.Component<TreeProps, any> {
+  constructor(props: any, context: any) {
+    super(props, context)
+    this.onClick = this.onClick.bind(this)


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 }

Nice, didn't know that!

BrendanAnnable · 2017-05-19T09:59:56Z

src/client/components/configuration/tree/tree.tsx

+
+    return (
+      <div className={classes}>
+        <div className={style.treenode__header} onClick={this.onClick} style={headerInlineStyle}>


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 🙂

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:

Hmmm, good point! You can sorta do it with pure CSS but it's pretty hacky.

Ah! Makes sense. Maybe add a comment explaining why then? 🙂 Otherwise the next person will think the same thing haha

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

Done: 6e431ac

- Also use the sidebar color for hover and selected states

JosephusPaye · 2017-06-27T08:30:27Z

Hey @BrendanAnnable,

After pulling in the latest master (1992968), I'm having trouble compiling the code.

Errors

ERROR in [at-loader] ./src/client/components/configuration/editor/editor.tsx:33:2
    TS2345: Argument of type 'typeof Editor' is not assignable to parameter of type 'string[]'.
  Property 'includes' is missing in type 'typeof Editor'.

ERROR in [at-loader] ./src/client/components/configuration/editor/editor.tsx:39:41
    TS2605: JSX element type 'MapField' is not a constructor function for JSX elements.

ERROR in [at-loader] ./src/client/components/configuration/editor/editor.tsx:40:42
    TS2605: JSX element type 'ListField' is not a constructor function for JSX elements.

ERROR in [at-loader] ./src/client/components/configuration/editor/editor.tsx:45:11
    TS2605: JSX element type 'ScalarField' is not a constructor function for JSX elements.

ERROR in [at-loader] ./src/client/components/configuration/editor/fields/scalar_field.tsx:22:13
    TS2605: JSX element type 'NumberValue' is not a constructor function for JSX elements.
  Types of property 'setState' are incompatible.
    Type '{ <K extends never>(f: (prevState: void, props: FieldProps) => Pick<void, K>, callback?: (() => a...' is not assignable to type '{ <K extends never>(f: (prevState: {}, props: any) => Pick<{}, K>, callback?: (() => any) | undef...'.
      Types of parameters 'f' and 'f' are incompatible.
        Type '(prevState: {}, props: any) => Pick<{}, any>' is not assignable to type '(prevState: void, props: FieldProps) => Pick<void, any>'.
          Types of parameters 'prevState' and 'prevState' are incompatible.
            Type 'void' is not assignable to type '{}'.

It seems to have something to do with observer from mobx-react.

I get no errors when I run yarn tslint. To test checkout my latest commit: 9135187.

Initial work on Configuration

9cc5f06

- Includes a basic working Tree component for browsing configuration files

BrendanAnnable suggested changes May 18, 2017

View reviewed changes

JosephusPaye added 4 commits May 19, 2017 11:44

Convert indents to 2-spaces

1f61d48

Add trailing newline to SVG files, use 2 space indents

a097af5

Remove trailing commas from interfaces

ead72c5

Remove default exports

5826601

BrendanAnnable suggested changes May 19, 2017

View reviewed changes

BrendanAnnable reviewed May 19, 2017

View reviewed changes

JosephusPaye added 14 commits May 22, 2017 17:29

Merge branch 'master' into feature/configuration

6fb0b90

Remove unnecessary tree constructor, render children only when expanded

b101805

Rename tree_store to tree_model

5188eac

Extract sidebar wrapper to standalone panel component

3e28be4

Extract view wrapper (and header) to dedicated view component

e01ab49

Add controller and model, move sample data to separate file

ab6b8a7

Use ul/li tags for tree

6e431ac

- Also use the sidebar color for hover and selected states

Add comment about inline paddingLeft

833fe7a

Tweak indentation comment

9d6f6a4

Merge origin/master

76087a8

Merge branch 'master' into feature/configuration

a82d104

Merge branch 'master' into feature/configuration

01b483b

Merge branch 'master' into feature/configuration

6477f18

WIP configuration editor

9135187

JosephusPaye added 3 commits June 30, 2017 20:56

Make the basic editor work, fix styling

8ef925b

Merge branch 'master' into feature/configuration

27f8cff

.

fc52617

[WIP] Configuration View #19

Are you sure you want to change the base?

[WIP] Configuration View #19

Conversation

JosephusPaye commented May 18, 2017

BrendanAnnable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanAnnable May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephusPaye May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephusPaye commented May 19, 2017

BrendanAnnable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephusPaye May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanAnnable May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanAnnable May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephusPaye May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanAnnable May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanAnnable May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrendanAnnable May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosephusPaye commented Jun 27, 2017 • edited Loading

BrendanAnnable May 19, 2017 •

edited

Loading

JosephusPaye May 19, 2017 •

edited

Loading

JosephusPaye May 24, 2017 •

edited

Loading

BrendanAnnable May 19, 2017 •

edited

Loading

BrendanAnnable May 19, 2017 •

edited

Loading

JosephusPaye May 24, 2017 •

edited

Loading

BrendanAnnable May 19, 2017 •

edited

Loading

BrendanAnnable May 19, 2017 •

edited

Loading

BrendanAnnable May 24, 2017 •

edited

Loading

JosephusPaye commented Jun 27, 2017 •

edited

Loading