Add a categories page, component, and category search support #1237

tofumatt · 2016-10-14T19:29:34Z

~~Still a WIP and missing some tests, though I'm gonna finish that soon.~~

~~Will need mozilla/addons#3588 to finish.~~

This adds category support to the frontend. It's a beast of a patch and I'm totally sorry. I owe all reviewers beer at Hawaii.

There are a few style issues that I will happily tweak afterward, but I thought getting this merged before it ballooned further was wise.

I also realised that there isn't a paginator for the categories search results. I can add that here but again would be happy to do it in a follow-up patch to save review bloat.

Screenshots:

r?

Fixes #3676, supports mozilla/addons-frontend#1237

Implement a new /v3/categories/ endpoint for our static categories. Fixes #3676, supports mozilla/addons-frontend#1237

EnTeQuAk · 2016-10-27T17:52:06Z

The API is now available at /api/v3/addons/categories/, e.g https://addons-dev.allizom.org/api/v3/addons/categories/ for -dev.

mstriemer

You've got a .DS_Store and it looks like the Fira fonts you added aren't being used?

I mostly skimmed the tests because I've been looking at this for a while.

mstriemer · 2016-11-02T17:21:36Z

src/amo/components/Categories.scss

+  padding: 0;
+}
+
+.Categories-listItem {


I think we've been using PascalCase-kebab-case in the past.

mstriemer · 2016-11-02T17:22:39Z

src/amo/components/Categories.scss

+  padding: 0;
+  width: 95%;
+
+  a {


I've been trying not to use bare element selectors like this but it seems that everyone else is fine with it. I'd prefer .Categories-link but it's up to you.

No your way is totally better; I've been trying for that too and just missed this one. It means less nesting which is nice.

mstriemer · 2016-11-02T17:23:48Z

src/amo/components/CategoryLink.js

@@ -0,0 +1,39 @@
+// import React, { PropTypes } from 'react';


💣💥 🔥

mstriemer · 2016-11-02T17:24:17Z

src/amo/components/SearchPage.js

 import SearchResult from './SearchResult';


-export class SearchPageBase extends React.Component {
+export default class SearchPage extends React.Component {


Yay, less redux in components! 👏

mstriemer · 2016-11-02T19:20:14Z

src/amo/containers/CategoriesPage.js

+  render() {
+    const { CategoriesComponent, addonType } = this.props;
+    return (
+      <div className="Categories-Page" ref={(ref) => { this.container = ref; }}>


Is this component needed? It looks like this could export connect(...)(Categories) if you want to keep the redux out of the Categories file.

Actually is this file used at all? It looks like Categories has the same asyncConnect() statement.

I created it solely because it seems like a page and I figured it should have a container in addition to a component.

That said you're right it can be simplified a lot, I've tidied it up where the container does the asyncConnect and the component doesn't, and removed the HTML from the container. That makes sense. 👍

mstriemer · 2016-11-02T19:57:13Z

src/core/containers/SearchPage.js

+  dispatch, page, api, auth = false, addonType, category, query,
+}) {
+  // If none of the optional search params are found, we aren't searching for
+  if ([addonType, category, query].filter((param) => {


I think I've seen this snippet three times now, it should be pulling from the state as I mentioned above.

mstriemer · 2016-11-02T19:58:09Z

src/core/components/Search/SearchResults.js

    } = this.props;

-    let searchResults;
-    let messageText;
+    const searchParamIsPresent = [


This could use the value from state, too.

mstriemer · 2016-11-02T19:59:21Z

src/core/containers/SearchPage.js

+  };
+  const doingSearch = Object.keys(queryStringMap).filter((queryKey) => {
+    return (location.query[queryKey] !== undefined &&
+      location.query[queryKey] === state.search[queryStringMap[queryKey]]);


Should this check the URL? Do we want this to be true at /en-US/firefox/addons/ublock-origin/?q=huh?

mstriemer · 2016-11-02T20:06:48Z

tests/client/amo/components/TestCategeories.js

+  {
+    application: 'android',
+    name: 'Games',
+    slug: 'games',


Should probably have some slugs that aren't just lowercase of the name to be safe.

mstriemer · 2016-11-02T20:09:32Z

tests/client/amo/containers/TestHome.js



 describe('Home', () => {
+  // function render(props) {


tofumatt · 2016-11-03T12:15:44Z

Oh, you’re right, in which case now I’m confused. I’ll check it out.

tofumatt · 2016-11-03T15:51:25Z

Well the lack of “s” is less code/confusion; the “s” is prettier URLs. But it’s just a .replace(/s$/, ‘') call right now, so I think it’s worth it.

tofumatt · 2016-11-03T18:48:46Z

Okay, updated things and I think I addressed all the comments. Ready for another r?

muffinresearch · 2016-11-07T11:27:55Z

Re: URLS, we can't deviate from the urls used on AMO until we've done desktop. I've filed mozilla/addons#9947 to check this along with the rationale behind it.

mstriemer

I only made it about half way through this but I think not using the search component for categories will make a lot of changes so I'm going to stop here for now.

Sorry I missed this the first time (and for the slow review this time), I think I saw the component and wasn't sure what it was for but didn't connect the dots to what was happening.

It seems like there's a lot of logic for categories and search that once combined with the logic to differentiate the two makes the search really bloated and hard to follow. I don't think splitting the two will cause much duplication, if any, and it should make the components much simpler.

mstriemer · 2016-11-07T20:30:49Z

.eslintrc

@@ -12,6 +12,7 @@
  },
  "parser": "babel-eslint",
  "rules": {
+    "arrow-body-style": ["off", "always"],


I'd generally say do this separately, especially since we had some discussion in an issue about it. Do you mind splitting this out into its own commit before you merge?

I think an update caused it to start yelling at me so I changed it (or maybe it was just a tonne of warnings).

I will do it if it doesn't cause failures though, sure thing.

mstriemer · 2016-11-07T20:32:36Z

src/amo/components/Categories.js

+
+
+export function filterAndSortCategories(categories, addonType, clientApp) {
+  return categories.filter((category) => {


I really prefer the () form here but I seem to be the only one :(

return categories.filter((category) => ( category.type === addonType && category.application === clientApp; )).sort((a, b) => a.name > b.name);

I find the implicit returns harder to follow and also just prefer all functions look the same for consistency. I find implicit return JS to be harder to scan.

mstriemer · 2016-11-07T20:34:12Z

src/amo/components/Categories.js

+      addonType, categories, clientApp, error, loading, i18n,
+    } = this.props;
+
+    if (loading && !categories.length) {


Should we always do this instead of only when we don't have categories? Why would we have categories and still be loading?

mstriemer · 2016-11-07T20:36:14Z

src/amo/components/Categories.js

+    } = this.props;
+
+    if (loading && !categories.length) {
+      return <div>{i18n.gettext('Loading...')}</div>;


Maybe these <div>s should have the Category class, and potentially a <p> inside of them. That way any general layout stuff is the same.

mstriemer · 2016-11-07T20:41:09Z

src/amo/components/Categories.js

+            return (
+              <li className="Categories-list-item">
+                <Link className="Categories-link"
+                  to={{ pathname: '/search/', query: queryParams }}>


Do we not have a category detail page? There's currently one at https://addons.mozilla.org/en-US/firefox/extensions/search-tools/ and I think this makes way more sense than going to search.

If there's an issue for this that's cool but I feel we should have a detail page that uses this search query in the background.

Yeah we should have that–there are URL parity bugs and this would be a case where I should add it.

mstriemer · 2016-11-07T20:48:26Z

src/amo/components/CategoryInfo.js

+      return null;
+    }
+
+    const categoryMatch = categories.filter((category) => {


It feels awkward to keep filtering the categories all over. Can you store them as { firefox: { extensions: { search: [] } } } instead?

Yeah, fair point.

mstriemer · 2016-11-07T20:49:38Z

src/amo/components/CategoryInfo.js

+      return (category.application === clientApp && category.slug === slug &&
+        category.type === addonType);
+    });
+    const category = categoryMatch.length ? categoryMatch[0] : false;


There's an Array.prototype.find() that would remove the need for this but restructuring how we store categories is probably better.

mstriemer · 2016-11-07T20:52:33Z

src/amo/components/SearchPage.js

        pathname="/search/" query={{ q: query }} showPages={0} /> : [];

    return (
      <div className="search-page">
-        <SearchResults results={results} query={query} loading={loading}
-          count={count} ResultComponent={SearchResult} />
+        <SearchResults CategoryInfoComponent={CategoryInfoComponent}


Category info doesn't feel related to search. I'd say it should be its own page that just calls search for its API request.

mstriemer · 2016-11-07T20:56:25Z

src/core/actions/search.js

@@ -4,23 +4,23 @@ import {
  SEARCH_FAILED,
 } from 'core/constants';

-export function searchStart(query, page) {
+export function searchStart({ page, query, addonType, app, category }) {


This really feels like it's modifying search too much.

tofumatt · 2016-11-07T21:13:36Z

Ah, I see. I figured extending the search and allowing it the flexibility to be the one place we'd look for add-ons would be the best option, allowing for really specific searches by both users and admins in the future.

We can totally split it into its own component with likely little duplication, just figured it might be cool to allow a search page with lots of query options.

Parts like including category info in the search page is fair enough, but I felt like any listing of add-ons was essentially a search and would make sense to be treated as one.

My gut reaction is I would rather thing extend search than become its own component. I'll have another look and see if I'm wrong on that and it'd be cleaner separated or if there's at least more than could be split out.

mstriemer · 2016-11-08T18:34:07Z

src/core/api/index.js

+  });
+}
+
+export function search({ api, page, query, auth = false, category, addonType }) {


I think I'd like this to be more generic for the extra filters. Essentially thinking of it as search({ query, page, filters }) and filters would have the category/addon type in it. Once that's done it might be nicer to wrap this in a category() function so it's clear how to get category listings.

export function category({ category, addonType, filters = {}, ...args }) { return search({ ...args, filters: { ...filters, category, addonType } }); }

mstriemer · 2016-11-08T19:14:55Z

src/core/reducers/categories.js

+
+
+const initialState = {
+  categories: [],


I commented earlier about this but I'll add something since we just discussed some things on IRC.

I think this should be more like:

{ categories: { adblocking: { firefox: { extensions: { count: 20, results: ['abp', 'ublock', ...], page: 1, }, }, }, abstract: { firefox: { themes: { count: 123897, results: ['blue', 'fractal', 'liberty', ...], page: 33, }, }, }, }, }

This is really nested but I'm not sure if there'd be a good way to flatten it. The bit inside extensions or themes could be a "sub-reducer" that is what we should be using as well in the search reducer. The order could be different too. themes.firefox.abstract or firefox.extensions.adblocking.

This lacks some style bits which I want to address later. This commit has become quite massive. Implements category component, category page container, and adds category support to search.

tofumatt · 2016-11-17T17:48:56Z

Superseded by mozilla/addons-server#1376, which is now merged.

tofumatt added the pull request ready label Oct 14, 2016

tofumatt force-pushed the implement-categories-1183 branch 2 times, most recently from 8ef6093 to 10925eb Compare October 24, 2016 19:50

EnTeQuAk mentioned this pull request Oct 25, 2016

Category List API mozilla/addons-server#3863

Merged

tofumatt force-pushed the implement-categories-1183 branch from 225bfb8 to 9f49530 Compare October 26, 2016 11:45

EnTeQuAk added a commit to mozilla/addons-server that referenced this pull request Oct 26, 2016

Implement a new /v3/categories/ endpoint for our static categories.

07a652f

Fixes #3676, supports mozilla/addons-frontend#1237

EnTeQuAk added a commit to mozilla/addons-server that referenced this pull request Oct 26, 2016

Category List API (#3863)

18d3e83

Implement a new /v3/categories/ endpoint for our static categories. Fixes #3676, supports mozilla/addons-frontend#1237

tofumatt force-pushed the implement-categories-1183 branch 2 times, most recently from 6f75ba9 to b86c20c Compare November 2, 2016 16:12

tofumatt changed the title ~~WIP: Add a categories page, component, and category search support~~ Add a categories page, component, and category search support Nov 2, 2016

tofumatt assigned mstriemer Nov 2, 2016

tofumatt force-pushed the implement-categories-1183 branch from b86c20c to 51bf303 Compare November 2, 2016 16:24

mstriemer suggested changes Nov 2, 2016

View reviewed changes

tofumatt mentioned this pull request Nov 3, 2016

Add featured API support (fix #1335) #1337

Merged

mstriemer suggested changes Nov 7, 2016

View reviewed changes

tofumatt force-pushed the implement-categories-1183 branch from 5202a01 to 48e7f81 Compare November 7, 2016 21:17

mstriemer reviewed Nov 8, 2016

View reviewed changes

tofumatt added 2 commits November 8, 2016 22:11

Implement categories (fixes #1183)

a76ff28

This lacks some style bits which I want to address later. This commit has become quite massive. Implements category component, category page container, and adds category support to search.

Improve reducers and actions

db84db9

tofumatt force-pushed the implement-categories-1183 branch from 48e7f81 to db84db9 Compare November 10, 2016 00:52

tofumatt added 3 commits November 10, 2016 01:28

Fix lint

88c9f89

Return sorted object with coorrect key

a075323

Don't change arrow brace style

e4d0eda

Change route

4e56f0d

tofumatt mentioned this pull request Nov 10, 2016

Add Category Page (close #1183) #1365

Merged

tofumatt closed this Nov 17, 2016

tofumatt deleted the implement-categories-1183 branch November 17, 2016 17:48

tofumatt removed the pull request ready label Nov 17, 2016

		@@ -0,0 +1,39 @@
		// import React, { PropTypes } from 'react';



		export function filterAndSortCategories(categories, addonType, clientApp) {
		return categories.filter((category) => {

Add a categories page, component, and category search support #1237

Add a categories page, component, and category search support #1237

Conversation

tofumatt commented Oct 14, 2016 • edited

Screenshots:

EnTeQuAk commented Oct 27, 2016 • edited

mstriemer 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

tofumatt commented Nov 3, 2016 via email

tofumatt commented Nov 3, 2016 via email

tofumatt commented Nov 3, 2016

muffinresearch commented Nov 7, 2016

mstriemer 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

tofumatt commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Nov 17, 2016

tofumatt commented Oct 14, 2016 •

edited

EnTeQuAk commented Oct 27, 2016 •

edited