Initial PR #4

JasonStoltz · 2018-08-20T18:28:23Z

Reverting a temporary commit, which deleted all code, in order to open up a PR.

This reverts commit cf25dc6.

scottybollinger

Happy to discuss some of these if you don't agree. Great work!

scottybollinger · 2018-08-20T18:33:30Z

public/index.html

+ <noscript>
+ You need to enable JavaScript to run this app.
+ </noscript>
+ <div id="root" class="app-container" \></div>


What is the \ for?

Looks like a typo that miraculously didn't blow things up.

scottybollinger · 2018-08-20T18:37:58Z

src/App.js

@@ -0,0 +1,57 @@
+import React, { Component } from "react";
+
+import Header from "./components/Header";


Just a suggestions, it might be a bit cleaner to export these from an index.js and have

import { Header, Body } from "./components"

Yes, I like that a lot.

scottybollinger · 2018-08-20T20:48:17Z

src/app-search/AppSearchDriver.js

+ sortField
+ };
+}
+


This could be simplified with destructuring

function filterSearchParameters({ current, filters, resultsPerPage, searchTerm, sortDirection, sortField }) { return { current, filters, resultsPerPage, searchTerm, sortDirection, sortField }; }

scottybollinger · 2018-08-20T21:12:12Z

src/app-search/AppSearchDriver.js

+ });
+ }
+ });
+ };


Is there any reason the server would ever respond with an error? If so, might be good to have a .catch here

Totally overlooked error handling, thanks.

scottybollinger · 2018-08-20T21:18:07Z

src/app-search/AppSearchDriver.js

+ searchTerm,
+ sortDirection,
+ sortField
+ });


Is there a reason you are destructuring and re-adding the entire state instead of:

removeFilter = (name, value) => { const { filters } = this.state; const updatedFilters = filters.filter(filter => !(filter[name] === value)); this._updateSearchResults({ ...this.state, current: 1, filters: updatedFilters }); };

I hadn't considered doing that. It's primarily because that method doesn't need the entire state, it just needs 5 fields from the state. It should be harmless to pass the extra fields though.

I could also possibly update the _updateSearchResults to simply do that piece of it inside of the method. So default to current state if a particular field isn't passed.

scottybollinger · 2018-08-20T22:15:42Z

src/config/engine.json.example

+ "sortFields": [
+ "title",
+ "anotherField"
+ ]


Missing comma

scottybollinger · 2018-08-20T22:16:19Z

src/containers/Facets.js

+
+import withAppSearch from "../app-search/withAppSearch";
+import Facets from "../components/Facets";
+import Facet from "../components/Facet";


Same here about index.js as above

scottybollinger · 2018-08-20T22:17:34Z

src/containers/Paging.js

+
+import withAppSearch from "../app-search/withAppSearch";
+
+// App search is currently limited to 100 pages, so we need to make sure


"App Search" (caps)

scottybollinger · 2018-08-20T22:19:32Z

src/containers/PagingInfo.js

+ totalResults <= resultsPerPage
+ ? totalResults
+ : start + resultsPerPage - 1;
+


I've always heard to have the operators at the end of the line. What about this?

const end = totalResults <= resultsPerPage ? totalResults : start + resultsPerPage - 1;

That may have been autoformatted, I'll have to check.

It was autoformatted that way, plus, I think I prefer that anyway.

scottybollinger · 2018-08-20T22:20:20Z

src/containers/Results.js

+
+import withAppSearch from "../app-search/withAppSearch";
+import Results from "../components/Results";
+import Result from "../components/Result";


Same here about index.js as above

JasonStoltz · 2018-08-21T13:49:20Z

I agree with all of your feedback, thanks! Going to make updates accordingly right now.

- Support config in test - index.js for components - destructure `filterSearchParameters` - Added error handling - Refactor _updateSearchResults - Changed all actions to call _updateSearchResults only with updated values. _updateSearchResults then defaults unprovided values to current state. - Expanded on driver param in provider - Explicitly specified the shape of the driver in PropTypes. - Simplified toSingleValue - URLManager code style changes - Added index.js for containers - Simplify map in jsx - Use template string in className - Reword comment to not say 'user' - Use brackets for long single line if - Use simple field assignment in config file - Missing comma - App Search with caps in comment

JasonStoltz · 2018-08-21T16:40:08Z

@scottybollinger Ready for a second look

scottybollinger · 2018-08-21T16:59:57Z

src/components/Facet.test.js

+ options={[{ value: "1", count: 1 }, { value: "2", count: 1 }]}
+ />
+ );
+ expect(wrapper).toMatchSnapshot();


This could be a little cleaner and DRY.

import React from "react"; import Facet from "./Facet"; import { shallow } from "enzyme"; const props = { name: "Facet", onRemove: () => {}, onSelect: () => {}, options: [{ value: "1", count: 1 }, { value: "2", count: 1 }] } it("renders correctly when a value is selected", () => { const wrapper = shallow(<Facet {...props} value="value" />); expect(wrapper).toMatchSnapshot(); }); it("renders correctly when a value is not selected", () => { const wrapper = shallow(<Facet {...props} />); expect(wrapper).toMatchSnapshot(); });

scottybollinger · 2018-08-21T17:00:38Z

src/components/Result.test.js

+import React from "react";
+import Result from "./Result";
+import { shallow } from "enzyme";
+


Same as above with share props object

scottybollinger · 2018-08-21T17:01:06Z

src/components/ResultsPerPage.test.js

+import ResultsPerPage from "./ResultsPerPage";
+import { shallow } from "enzyme";
+
+it("renders correctly when there is a selected value", () => {


Same as above with share props object

scottybollinger · 2018-08-21T17:01:19Z

src/components/SearchBox.test.js

+import React from "react";
+import SearchBox from "./SearchBox";
+import { shallow } from "enzyme";
+


Same as above with share props object

scottybollinger · 2018-08-21T17:01:29Z

src/components/Sorting.test.js

+import React from "react";
+import Sorting from "./Sorting";
+import { shallow } from "enzyme";
+


Same as above with share props object

scottybollinger · 2018-08-21T17:03:40Z

src/app-search/AppSearchDriver.js

+ sortField
+ } = {
+ ...this.state,
+ ...searchParameters


TIL this. ☝ Nicely done!

Me too ¯_(ツ)_/¯

scottybollinger

LGTM other than the test DRY stuff. 👍

JasonStoltz · 2018-08-21T17:25:22Z

@scottybollinger Good call on cleaning those tests up. Ready for another look!

Revert "Deleted everything"

4f67a50

This reverts commit cf25dc6.

JasonStoltz assigned scottybollinger Aug 20, 2018

scottybollinger suggested changes Aug 20, 2018

View reviewed changes

Added basic, component level tests

0aabca0

JasonStoltz force-pushed the review branch from 05d9f5b to 0aabca0 Compare August 21, 2018 14:24

JasonStoltz force-pushed the review branch from 2923889 to abaa5a7 Compare August 21, 2018 16:38

scottybollinger reviewed Aug 21, 2018

View reviewed changes

PR Feedback: DRY up tests

f4171a6

scottybollinger approved these changes Aug 21, 2018

View reviewed changes

JasonStoltz merged commit f17fd2d into master Aug 21, 2018

JasonStoltz deleted the review branch August 21, 2018 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Initial PR #4

Initial PR #4

Uh oh!

JasonStoltz commented Aug 20, 2018

scottybollinger left a comment

scottybollinger Aug 20, 2018

JasonStoltz Aug 21, 2018

scottybollinger Aug 20, 2018

JasonStoltz Aug 21, 2018

scottybollinger Aug 20, 2018

scottybollinger Aug 20, 2018

JasonStoltz Aug 21, 2018

scottybollinger Aug 20, 2018

JasonStoltz Aug 21, 2018

scottybollinger Aug 20, 2018

scottybollinger Aug 20, 2018

scottybollinger Aug 20, 2018

scottybollinger Aug 20, 2018

JasonStoltz Aug 21, 2018

JasonStoltz Aug 21, 2018

scottybollinger Aug 20, 2018

JasonStoltz commented Aug 21, 2018

JasonStoltz commented Aug 21, 2018

scottybollinger Aug 21, 2018

scottybollinger Aug 21, 2018

scottybollinger Aug 21, 2018

scottybollinger Aug 21, 2018

scottybollinger Aug 21, 2018

scottybollinger Aug 21, 2018

JasonStoltz Aug 21, 2018

scottybollinger left a comment

JasonStoltz commented Aug 21, 2018

Labels

3 participants

		@@ -0,0 +1,57 @@
		import React, { Component } from "react";

		import Header from "./components/Header";


		import withAppSearch from "../app-search/withAppSearch";

		// App search is currently limited to 100 pages, so we need to make sure

Uh oh!

Initial PR #4

Initial PR #4

Uh oh!

Conversation

JasonStoltz commented Aug 20, 2018

scottybollinger left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz commented Aug 21, 2018

JasonStoltz commented Aug 21, 2018

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

scottybollinger left a comment

Choose a reason for hiding this comment

JasonStoltz commented Aug 21, 2018

Labels

3 participants