Create component for Ingredient Search Box #28

vreddi · 2018-06-01T04:55:27Z

No description provided.

src/base.scss

@@ -7,8 +7,6 @@ html {
        width: 100%;
        height: 100%;
        margin: 0;
-        font-family: 'Source Sans Pro', sans-serif;


sukritchhabra · 2018-06-01T05:40:22Z

src/components/SearchBox/SearchBox.jsx

+import './SearchBox.scss';
+
+class SearchBox extends Component {
+  constructor(props) {


We can lose the constructor here and just assign state like this:

class SearchBox extends Component { state = { value: '', } }

I'll still keep the constructor for clarity.

What do you mean by clarity? Your constructor is literally doing nothing that can't be done without the constructor! It's just bloat at this point...

If there's something that cant be done without the constructor then it makes sense to use the constructor

Babel adds the constructor in the end after the transpile and injects super with all the properties (including props). For that explicit clarity I mentioned constructor to avoid confusions but I am fine with removing it.

sukritchhabra · 2018-06-01T05:41:48Z

src/components/SearchBox/SearchBox.jsx

+      value: '',
+    };
+
+    this.onInputChange = this.onInputChange.bind(this);


Once we lose the constructor this function will automatically be bound to the context of the class and this manual bind will become unnecessary.

sukritchhabra · 2018-06-01T05:43:43Z

src/components/SearchBox/SearchBox.jsx

+    this.onInputChange = this.onInputChange.bind(this);
+  }
+
+  onInputChange(event) {


Keep function definitions consistent, I think it makes sense to keep things consistent and use the es6 function definition format here. Will also save us 3-4 lines of code.

onInputChange = ({ target: { value } }) => this.setState({ value })

Sure - in that case the above comment is not needed.

sukritchhabra · 2018-06-01T05:44:53Z

src/components/SearchBox/SearchBox.jsx

+    const { value } = this.state;
+
+    return (
+      <div className="searchbox">


Inconsistent class naming. I think you use hyphenated classNames everywhere, why use camel case here?

Update: Sorry i meant why use lower case?

I probably missed that

vreddi · 2018-06-02T17:32:01Z

src/components/SearchBox/SearchBox.scss

+    $font-family: Roboto Slab, sans-serif;
+
+    position: relative;
+    width: 500px;


vreddi · 2018-06-02T06:32:31Z

src/components/SearchBox/SearchBox.scss

+
+    position: relative;
+    width: 500px;
+    margin: 50px auto;


sukritchhabra · 2018-06-01T05:58:12Z

src/components/SearchBox/SearchBox.scss

+
+        &:focus {
+            + .input-highlight {
+                border-top: 3px solid #8162FF;


lets make the color here also a variable?

same reason as above

sukritchhabra · 2018-06-01T05:59:24Z

src/components/SearchBox/SearchBox.jsx

+          value={value}
+          spellCheck={false}
+        />
+        <span className="input-highlight">


I like this but why do we need this? It would make sense if there was a character limit and work as a visual aid but right now its nothing but a glorified cursor

The purpose is aesthetics and design to shape the character of the ui. Anyways its not the final design. The project this is a part of is just a first pass at components. Other projects will cover the next phases

Im fine with this for now, but I don't think it serves any additional purpose. It may even be confusing as to what this is and what it's trying to tell a user.

Also there's a bug with this. The color doesn't persist onBlur.

vreddi · 2018-06-02T06:35:18Z

src/components/SearchBox/SearchBox.scss

+        font-family: $font-family;
+        overflow: hidden;
+    }
+}


Fixed. @sukritchhabra need to fix the linter for scss files.

I've been looking into it and I don't think we need linting cause our webpack build takes care of breaking builds... Also css is very specific in terms of the key value pairs it requires and anything out of that scope causes the element to not be styled properly, so dont think a linter will serve any additional purpose... As far as the indentation is concerned, use the same as the rest of the project...

vreddi · 2018-06-02T06:35:30Z

src/components/SearchBox/SearchBox.jsx

@@ -0,0 +1,50 @@
+import React, { Component } from 'react';


vreddi · 2018-06-02T17:11:09Z

.eslintrc.json

@@ -9,6 +9,8 @@
    "airbnb"
  ],

+  "parser": "babel-eslint",


To make class member functions es6 style.

It's not needed... You can do it without it.

@sukritchhabra It is. ES6 Class functions are not in the rules of basic es-lint. Need a babel wrapper for that.

Ah, yes, you're right! I uninstalled it locally not globally while checking...

sukritchhabra · 2018-06-02T20:43:40Z

src/assets/styles/colors.scss

+$colors-purple: #44318D;
+$colors-light-purple: #8162FF;
+$colors-light-gray-0: #757575;
+$colors-light-gray-1: #999;


Incorrect line ending

sukritchhabra · 2018-06-02T20:44:50Z

src/components/SearchBox/SearchBox.jsx

-    this.state = {
-      value: '',
-    };
+  onInputChange = ({ target: { value } }) => (


Unnecessary parenthesis around this.setState, can all be condensed to one line.

sukritchhabra · 2018-06-02T20:47:15Z

src/assets/styles/colors.scss

+$colors-purple: #44318D;
+$colors-light-purple: #8162FF;
+$colors-light-gray-0: #757575;
+$colors-light-gray-1: #999;


Lets follow this pattern for partials defined by scss and name the file _colors.scss

sukritchhabra · 2018-06-02T20:51:36Z

webpack.config.js

@@ -70,7 +70,7 @@ module.exports = {
    alias: {
      components: path.resolve(__dirname, 'src/components'),
      state: path.resolve(__dirname, 'src/state'),
-      styles: path.resolve(__dirname, 'src/components/styles'),
+      styles: path.resolve(__dirname, 'src/assets/styles'),


I realize we don't have a components/styles folder anymore but I think naming this folder styles is confusing... Technically assets should never hold any styles... Can we rename this to something more specific like variables / global-styles

I'd still rather keep them under assets to avoid more root nodes. Also would like to treat assets more than just a dir for images. Common styles and colors can be treated as an asset.

Oh we should def keep it under assets but just rename it to something other than styles

If its global-styles in assets I feel it is redundant. A dir called styles in assets is by default for a global need - to be used across the project.

yeah good point, lets name it something else then

styles make sense, why would you name it something else?

because it's not holding styles! What makes it more confusing is that you also call the alias styles which you use within our actual styles!

Create component for Ingredient Search Box, resolves #18

566fa40

vreddi added the task Booked work for the product label Jun 1, 2018

sukritchhabra requested changes Jun 1, 2018

View reviewed changes

sukritchhabra reviewed Jun 1, 2018

View reviewed changes

Resolving CR comments

671b001

sukritchhabra reviewed Jun 2, 2018

View reviewed changes

Resolving some more CR comments

37ea972

sukritchhabra reviewed Jun 2, 2018

View reviewed changes

vreddi added 3 commits June 2, 2018 13:54

Resolving some more PR comments

e3b08de

Fix line ending

fa3ca08

CR Comments

0cb1568

sukritchhabra approved these changes Jun 2, 2018

View reviewed changes

vreddi merged commit 5f3c60d into develop Jun 2, 2018

vreddi deleted the vreddi/AutoCompleteSearchBox branch June 2, 2018 22:06

Create component for Ingredient Search Box #28

Create component for Ingredient Search Box #28

Conversation

vreddi commented Jun 1, 2018

This comment was marked as resolved.

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

sukritchhabra Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

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

sukritchhabra Jun 2, 2018 • 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

vreddi Jun 2, 2018 • 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

sukritchhabra Jun 1, 2018 •

edited

Loading

sukritchhabra Jun 2, 2018 •

edited

Loading

vreddi Jun 2, 2018 •

edited

Loading