Displaying all accounts, showing individual account info on click, ba… #2

CallawayJay · 2018-04-24T22:38:04Z

…ck button functionality.

ChuckJonas · 2018-04-24T22:39:07Z

src/app.tsx

@@ -7,6 +8,7 @@ import { generateSelect } from 'ts-force';

 interface AppState {
  acc: AccountFields;
+  accs: any;


type this please

ChuckJonas · 2018-04-24T22:40:51Z

src/app.tsx

  }

  public render() {
-    return <AccountDisplay acc={this.state.acc} />;


curious why you removed this? I would expect instead there to be some sort of conditional rendering which determines if Search vs Display is shown

I see now that you've instead chosen to render <AccountDisplay /> from the ` component... Refactor so it's conditionally rendered here

ChuckJonas · 2018-04-24T22:41:24Z

src/components/accountDisplay.tsx

@@ -6,6 +6,7 @@ import { hot } from 'react-hot-loader';

 interface AccountDisplayProps {
  acc: AccountFields;
+  onBackClick: (account: AccountFields) => void;


not sure it makes sense to pass the account in

ChuckJonas · 2018-04-24T22:43:25Z

src/components/accountDisplay.tsx

@@ -14,25 +15,33 @@ export class AccountDisplay extends React.Component<AccountDisplayProps, {}> {
    super(props);
  }

+  public handleBackClick = () => {
+    this.props.onBackClick(null);


again, this is somewhat weird. I don't think the back button should be responsible for setting a value.

ChuckJonas · 2018-04-24T22:43:59Z

src/components/accountDisplay.tsx

      return <div>No Account Found</div>;
    }

    return (
-      <Card title={this.props.acc.name}>


you don't need to change any of the display of this component for the assignment

ChuckJonas · 2018-04-24T22:44:55Z

src/components/searchControls.tsx

@@ -0,0 +1,30 @@
+import { Button, Form, Input } from 'antd';
+import * as React from 'react';
+import { hot } from 'react-hot-loader';


import is not needed

ChuckJonas · 2018-04-24T22:50:22Z

src/components/searchResults.tsx

+          return <div>No Accounts Found</div>;
+        }
+
+        if (this.state.selectedAccount) {


I don't think it makes sense for the Search Results component to be responsible for rendering the selected account.

1: this just doesn't really fit into the component hierarchy. EG: it wouldn't really make sense to render the account here as then the search controls would still be show.

2: It add additional dependancies. Search Results should only be responsible for displaying the results... not displaying the selected account. Once an account is "Selected" the interface should just replace the search components with the AccountDisplay component.

ChuckJonas · 2018-04-24T22:51:42Z

src/components/searchResults.tsx

+}
+
+interface SearchResultsStates {
+    selectedAccount: AccountFields;


I think this state should be managed higher up in the component tree (related to below comment about how the search results component shouldn't be responsible for rendering the AccountDisplay)

ChuckJonas · 2018-04-24T23:03:20Z

src/components/searchResultsItem.tsx

+    onAccountClick?: (account: AccountFields) => void;
+}
+
+export class SearchResultsItem extends React.Component<SearchResultsItemProps, {}> {


this is a good case for a "Stateless function" component

ChuckJonas · 2018-04-24T23:03:36Z

src/components/searchResults.tsx

+import { Account, AccountFields} from '@src/generated/sobs';
+import { Form } from 'antd';
+import * as React from 'react';
+import { hot } from 'react-hot-loader';


import not needed

…e been inputted

ChuckJonas

@CallawayJay A couple minor tweaks needed but overall looking good. Code is well structured and properties are named correctly.

Demonstrates Knowledge of:

Controlled Inputs
Conditional rendering
Passing State modification actions through component hierarchy

There is still room for improvement around

how functions behave as parameters
Typescript types
Use of Stateless Function components

ChuckJonas · 2018-04-25T16:35:46Z

src/components/searchDisplay.tsx

+
+interface SearchDisplayStates {
+    searchValue: string;
+    searchResults: any;


never use any type if you don't have to... This can easily be typed

ChuckJonas · 2018-04-25T16:37:50Z

src/components/searchDisplay.tsx

+                        <Input 
+                            id='search-box'
+                            value={this.state.searchValue}
+                            onChange={evt => this.updateSearchValue(evt)}


this is redundant. Because you aren't changing the parameter in anyway, just pass in the function (instead of calling updateSearchValue in a inline function).

this is the equivent of

function(evt){ this.updateSearchValue(evt); }

ChuckJonas · 2018-04-25T17:07:15Z

src/components/searchDisplay.tsx

+                    }
+
+                </Form>
+                <ul>


Probably should have been decomposed into it's own component just to improve decomposition

…larity. And make other tweaks for best practices.

Displaying all accounts, showing individual account info on click, ba…

ae73608

…ck button functionality.

ChuckJonas suggested changes Apr 24, 2018

View reviewed changes

ChuckJonas reviewed Apr 24, 2018

View reviewed changes

JayCogen added 2 commits April 25, 2018 10:33

Adding search functionality

842e4cc

Adding functionality to show search button only when 3 characters hav…

dd01ea5

…e been inputted

ChuckJonas suggested changes Apr 25, 2018

View reviewed changes

ChuckJonas reviewed Apr 25, 2018

View reviewed changes

JayCogen added 2 commits May 7, 2018 00:12

Broke the search display component our into stateless functions for c…

2a35772

…larity. And make other tweaks for best practices.

Breaking out SearchDisplay functions into stateless components

d43ea1f

+                                  }
+                              </Form>
+                              <ul>

Displaying all accounts, showing individual account info on click, ba… #2

Are you sure you want to change the base?

Displaying all accounts, showing individual account info on click, ba… #2

Uh oh!

Conversation

CallawayJay commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChuckJonas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!