Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create component for Ingredient Search Box #28

Merged
merged 6 commits into from
Jun 2, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
"airbnb"
],

"parser": "babel-eslint",

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

To make class member functions es6 style.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


"parserOptions": {
"ecmaFeatures": {
"experimentalObjectRestSpread": true,
Expand Down
209 changes: 209 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
},
"devDependencies": {
"babel-core": "^6.26.3",
"babel-eslint": "^8.2.3",
"babel-jest": "^21.2.0",
"babel-loader": "^7.1.4",
"babel-preset-es2015": "^6.24.1",
Expand Down
4 changes: 4 additions & 0 deletions src/assets/styles/colors.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
$colors-purple: #44318D;
$colors-light-purple: #8162FF;
$colors-light-gray-0: #757575;
$colors-light-gray-1: #999;
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect line ending

Copy link
Contributor

@sukritchhabra sukritchhabra Jun 2, 2018

Choose a reason for hiding this comment

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

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

1 change: 1 addition & 0 deletions src/base.scss
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ html {
width: 100%;
height: 100%;
margin: 0;
font-family: 'Source Sans Pro', sans-serif;

#content {
width: 100%;
Expand Down
4 changes: 3 additions & 1 deletion src/components/Header/Header.scss
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
@import '~styles/colors';

.header {
display: flex;
justify-content: flex-end;
overflow: hidden;
background: #44318D;
background: $colors-purple;
padding: 10px 30px;

.header-user-circle {
Expand Down
28 changes: 10 additions & 18 deletions src/components/SearchBox/SearchBox.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,31 +3,23 @@ import PropTypes from 'prop-types';
import './SearchBox.scss';

class SearchBox extends Component {
constructor(props) {
super();
state = {
value: '',
};

this.props = props;
this.state = {
value: '',
};
onInputChange = ({ target: { value } }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

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

this.setState({ value })
);

this.onInputChange = this.onInputChange.bind(this);
}

onInputChange(event) {
const { value } = event.target;

this.setState({ value });
}

render() {
render = () => {
const { value } = this.state;
const { placeholder } = this.props;

return (
<div className="searchbox">
<div className="search-box">
<input
onChange={this.onInputChange}
placeholder={this.props.placeholder}
placeholder={placeholder}
value={value}
spellCheck={false}
/>
Expand Down
20 changes: 11 additions & 9 deletions src/components/SearchBox/SearchBox.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
.searchbox {
@import '~styles/colors';

.search-box {
$input-font-size: 30px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to use em's so we can handle responsive changes easily in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think we should move our scss variables out into a separate file so that they can be imported anywhere and we can change them in one place if need be.

Copy link
Member Author

Choose a reason for hiding this comment

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

These variables are local to this file. The variables defined here are just intended for this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the em's. There is a separate bug that will cover px -> em's. #29

$input-line-height: 70px;
$font-family: Roboto Slab, sans-serif;
Expand All @@ -15,24 +17,24 @@
border-radius: 0;
line-height: $input-line-height;
background-color: transparent;
color: #757575;
color: $colors-light-gray-0;
font-size: $input-font-size;
border: none;
outline: none;
border-bottom: 3px solid #757575;
border-bottom: 3px solid $colors-light-gray-0;
font-family: $font-family;

::placeholder {
color: $colors-light-gray-1;
}

&:focus {
+ .input-highlight {
border-top: 3px solid #8162FF;
border-top: 3px solid $colors-light-purple;
}
}
}

::placeholder {
color: #999;
}

.input-highlight {
font-size: $input-font-size;
user-select: none;
Expand All @@ -47,4 +49,4 @@
font-family: $font-family;
overflow: hidden;
}
}
}
2 changes: 1 addition & 1 deletion webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@vreddi vreddi Jun 2, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah good point, lets name it something else then

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

images: path.resolve(__dirname, 'src/assets/images'),
},
extensions: ['.js', '.jsx'],
Expand Down