Create a desktop search page #2618

tofumatt · 2017-06-21T12:39:40Z

This one was a bit intense! I cleaned up the folder structure and ended up refactoring/cleaning up the SearchResult CSS. Pretty much everything here is a style change except the new SearchContextCard at the top of the page which shows result counts, etc.

We had, in my opinion, more than enough space for add-on descriptions in the medium/"tablet" view so I added them. It felt okay on a tablet, cue "developers hate whitespace" jokes.

Here is a bunch of screenshots:

Small

Medium

Large/Desktop

muffinresearch · 2017-06-21T12:48:36Z

src/amo/components/AddonsCard/styles.scss

+@import "~ui/css/vars";
+
+@include respond-to(large) {
+  ul.AddonsCard-list {


Is it possible to factor out the ul?

muffinresearch · 2017-06-21T12:50:33Z

src/amo/components/AddonsCard/styles.scss

+@import "~core/css/inc/mixins";
+@import "~ui/css/vars";
+
+ul.AddonsCard-list {


Is it possible to factor out the ul?

I can have a look!

muffinresearch · 2017-06-21T12:51:31Z

src/amo/components/Search/styles.scss

+    }
+  }
+
+  .SearchSort {


Do these need to be nested?

They do in order to be scoped to this particular container; I didn't want the SearchSort to have these styles by default–I figured with things like margins we can set them in the container's styles scoped like this so different containers could position it slightly differently.

That said maybe I could just pass in a className so in general I'll see about improving the nesting here.

muffinresearch · 2017-06-21T12:51:43Z

src/amo/components/Search/styles.scss

+    padding: $padding-page;
+  }
+
+  .Card {


Is this an override of something generic?

It adds padding to the cards on the search page, but it might be possible to do without the nesting, I'll see.

muffinresearch · 2017-06-21T13:02:15Z

src/amo/components/SearchSort/SearchSort.scss

@@ -65,6 +73,10 @@
    background: transparentize($link-color, 0.8);
    box-shadow: none;
  }
+
+  @include respond-to(large) {


Any time we're using display: none we should also consider the alternative of with-holding the content.

Of course changing content output based on UA or the mamo cookie is not equal to a viewport size based decision. See also https://github.com/mozilla/addons-frontend/issues/2355 (this is probably something for later rather than now).

The issue with that is especially as we're using viewports that aren't tied to device sizes but just sane breakpoints... you'd end up without content when you turned your phone around.

In general I'd say "we should only load important content and if we're hiding a LOT of stuff entirely except desktop... yes maybe it isn't needed". But unless we can say a specific component should only load with a certain UA... I kinda dislike loading different sets of content. Take, for example, the search page on a Nexus 6 in portrait versus landscape:

Portrait

Landscape

Landscape (w/o content loaded)

I think there's enough room to show the content and furthermore we should load it to prevent weirdness with different UAs getting/not getting content. I think that will make things harder to QA/test and debug, in addition to being a worse UX.

Yep, I completely agree about the trade-offs that's why I mentioned the header approach is not equal to a viewport size based decision. Especially since the header available has no granularity say compared to using a device database to customize responses to the device more closely.

One place where with-holding content might be a benefit in the future is if significant page weight could be saved on mobile as opposed to just hiding the content via styling.

This is something that can be considered on a case-by-case and as with any optimizations we can look at it later when we're specifically looking at ways to reduce page size / increase perf.

Yeah, I thought that too... with sagas we can have the notion of server-versus-client-side sagas. One way to do this might be to have the UA and viewport be stored in state and only fire off actions/saga requests for certain bits of data if the viewport is big enough.

I think that's only useful if the perf gain outweighs all the complexities that go with such a solution, but I'd be okay with that.

muffinresearch · 2017-06-21T13:03:24Z

src/ui/components/CardList/styles.scss

@@ -11,7 +14,7 @@
    padding: 0;

    > li {


Side note: Would be nice to swap things like this out for distinct classes when the opportunity presents itself.

kumar303

Looks good. I had some minor cleanup requests and some questions.

kumar303 · 2017-06-22T14:55:10Z

src/amo/components/Search/index.js

@@ -71,3 +89,8 @@ export default class SearchPage extends React.Component {
    );
  }
 }
+
+export default compose(
+  safeAsyncConnect([{ promise: loadSearchResultsIfNeeded }]),


Good call on not porting this to a saga all in one patch. It would have been harder to review :)

Yeah porting Search to saga will be awesome but later is a good idea. It'll require a lot of UX tweaks 😄

kumar303 · 2017-06-22T15:12:03Z

src/amo/components/SearchResult/index.js

+    // Fall-back to default icon if invalid icon url.
+    const iconURL = isAllowedOrigin(addon.icon_url) ? addon.icon_url : fallbackIcon;
+    const themeURL = (addon.theme_data && isAllowedOrigin(addon.theme_data.previewURL)) ?
+      addon.theme_data.previewURL : null;


thank you for not using addon.themeURL 😁

kumar303 · 2017-06-22T15:16:42Z

src/amo/components/SearchResult/index.js

+            <h2 className="SearchResult-name">{addon.name}</h2>
+            <p
+              className="SearchResult-summary"
+              dangerouslySetInnerHTML={sanitizeHTML(addon.summary)}


Does this need to fall back to addon.description for themes?

It probably should... really I wonder if we should be doing that in the reducer.

Filed #2626.

Though I opted not to use description here because it has the potential to be very long and I don't want all that text on the search results page. We will deal with it better in #2626 where if we find a description but no summary we can make a shortened summary from description I think. But for now they are fine blank.

kumar303 · 2017-06-22T15:34:59Z

src/amo/containers/CategoryPage.js

 import { loadByCategoryIfNeeded, parsePage } from 'core/searchUtils';
 import { apiAddonType, safeAsyncConnect } from 'core/utils';


 export function CategoryPageBase(props) {
-  return <SearchPage enableSearchSort={false} {...props} />;
+  return <SearchBase enableSearchSort={false} {...props} />;


Using SearchBase is a red flag to me because it means that the wrapped component is not generic enough. I think you need to remove safeAsyncConnect from amo/components/Search and make a new component that will load data. If you don't do this, the patch to switch to a saga will be more complicated; it will need conditional logic in componentWillMount to know if it should load data or not. Thus, that suggests to me that we need two components.

I don't quite follow: that's already how we switched from Category and it was pretty straightforward; once we switch to saga it would be a matter of checking in SearchBase for the search results given the filter set. If that component always is set up to dispatch when it doesn't have the right search data things will be good as pages like amo/components/Category also use the SearchBase component to render out their data.

To be the separate component used to load the data is the one we export using compose. I don't think it should go in a separate file though.

kumar303 · 2017-06-22T15:37:14Z

src/amo/containers/SearchPage.js

-
-
-export default compose(
-  safeAsyncConnect([{ promise: loadSearchResultsIfNeeded }]),


For the reasons mentioned above, I think you still need this as a separate component. It doesn't have to be a container, just a separate component.

kumar303 · 2017-06-22T15:38:20Z

src/core/css/inc/mixins.scss

@@ -1,6 +1,6 @@
 @import './vars';

-/* Font mixins. Changes here affect *alL* apps */
+/* Font mixins. Changes here affect *all* apps */


kumar303 · 2017-06-22T15:39:30Z

tests/unit/amo/components/TestLandingPage.js

 import { getFakeI18nInst } from 'tests/unit/helpers';


 describe('<LandingPage />', () => {
-  const initialState = { api: { clientApp: 'android', lang: 'en-GB' } };


kumar303 · 2017-06-22T18:17:55Z

tests/unit/amo/components/TestSearchContextCard.js

+
+    return shallow(
+      <SearchContextCardBase
+        {...mapStateToProps(store.getState())}


This is interesting. I think Enzyme gives us an opportunity to stop using the base component (which was always a hack).

However, HOC rendering isn't supported very well yet.

Until it is, we could do it like this (I tested this locally). It's a bit hacky, what do you think? By rendering the real component chain I think we will get better coverage.

diff --git a/tests/unit/amo/components/TestSearchContextCard.js b/tests/unit/amo/components/TestSearchContextCard.js index 6504093e..5e8861f4 100644 --- a/tests/unit/amo/components/TestSearchContextCard.js +++ b/tests/unit/amo/components/TestSearchContextCard.js @@ -1,10 +1,7 @@ import { shallow } from 'enzyme'; import React from 'react'; -import { - SearchContextCardBase, - mapStateToProps, -} from 'amo/components/SearchContextCard'; +import SearchContextCard from 'amo/components/SearchContextCard'; import { searchLoad, searchStart } from 'core/actions/search'; import { dispatchClientMetadata, fakeAddon } from 'tests/unit/amo/helpers'; import { getFakeI18nInst } from 'tests/unit/helpers'; @@ -19,13 +16,16 @@ describe('SearchContextCard', () => { store.dispatch(searchStart({ filters: { query: 'test' } })); } - return shallow( - <SearchContextCardBase - {...mapStateToProps(store.getState())} + const options = { context: { store } }; + + const root = shallow( + <SearchContextCard i18n={getFakeI18nInst()} {...props} - /> - ); + />, options + ) + + return root.first().shallow(options).first().shallow(options); } it('should render a card', () => {

If we add a new HOC to the chain, the test code will need an update but only in one place.

I was with you until return root.first().shallow(options).first().shallow(options); 😆

I suppose we could write a helper for that, although I liked importing things directly from enzyme, it felt so clean!

What happens when we have multiple things in the compose chain like a connect() call and a translate() call?

You will need to call .first().shallow() for each function passed to compose. I was thinking we could maybe do it in a less hacky way with a pattern like this:

// in the component: export wrappers = [ translate(), connect(mapStateToProps), ]; export default compose(...wrappers)(MyComponent); // In the test: import MyComponent, { wrappers } from 'src/MyComponent.js' // In render(): const root = unwrap(<MyComponent {...props} />, { wrappers }); // in unwrap(): let root = root.shallow(); // top-level wrappers.forEach(() => { // Unwrap each HOC: root = root.first().shallow(); });

I think that's a great approach. 👍

I'd be happy to go through all the components that need it and do it in one PR rather than in this one (it's already pretty big). But let me know if you'd rather me do it here.

Let's do it in https://github.com/mozilla/addons-frontend/issues/2643

kumar303 · 2017-06-22T18:41:46Z

tests/unit/amo/components/TestSearchContextCard.js

+
+  it('should use singular form when only one result is found', () => {
+    const { store } = dispatchClientMetadata();
+    store.dispatch(searchLoad({


It looks like you could replace this call with a helper and save quite a bit of code. In other words, the result part can be calculated automatically with a call like:

dispatchSearchResults({ store, addons: { [fakeAddon.slug]: fakeAddon }, filters: { query: 'test' }, });

kumar303 · 2017-06-22T18:45:30Z

tests/unit/amo/components/TestSearchResult.js

+
+  function render({ addon = baseAddon, lang = 'en-GB', ...props } = {}) {
+    return shallow(
+      <SearchResultBase


I don't think you need to use the base component. All I see is a translate() wrapper so you could use the unwrapping approach above and test the final component directly.

tofumatt · 2017-06-23T12:09:57Z

As we discussed on IRC I think the component setup here is okay for now and shouldn't make Saga refactoring complicated, so this is ready for another r? if that's okay. Let me know if there's still stuff you think should be refactored but I think this should be good and it will help make #2625 easier to land too.

kumar303

Ship it! I filed https://github.com/mozilla/addons-frontend/issues/2643 to deal with the HOC unwrapping later.

chore: Rename SearchPage to Search and consolidate

0158cd6

tofumatt requested a review from kumar303 June 21, 2017 12:39

tofumatt added the state:pull_request_ready label Jun 21, 2017

tofumatt force-pushed the search-desktop-2560 branch 3 times, most recently from 9b89dd8 to dd6282f Compare June 21, 2017 12:48

feat: Add desktop view for search (fix #2560)

4a65e24

tofumatt force-pushed the search-desktop-2560 branch from dd6282f to 4a65e24 Compare June 21, 2017 12:49

muffinresearch reviewed Jun 21, 2017

View reviewed changes

kumar303 suggested changes Jun 22, 2017

View reviewed changes

tofumatt mentioned this pull request May 5, 2024

Use description/summary as fallback for each other in reducer mozilla/addons#10503

Closed

tofumatt added 2 commits June 23, 2017 12:56

chore: Refactor search dispatch helper

86f240b

chore: Use description if summary is not available

33d963c

tofumatt requested a review from kumar303 June 23, 2017 12:10

fix: Actually do not use description if it is too long

0f41a6e

kumar303 mentioned this pull request Jun 27, 2017

Unwrap HOC components to make shallow render tests more effective mozilla/addons#10515

Closed

kumar303 approved these changes Jun 27, 2017

View reviewed changes

tofumatt merged commit 829521b into master Jun 27, 2017

tofumatt deleted the search-desktop-2560 branch June 27, 2017 22:35

tofumatt removed the state:pull_request_ready label Jun 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a desktop search page #2618

Create a desktop search page #2618

tofumatt commented Jun 21, 2017

muffinresearch Jun 21, 2017

muffinresearch Jun 21, 2017

tofumatt Jun 21, 2017

muffinresearch Jun 21, 2017

tofumatt Jun 21, 2017

muffinresearch Jun 21, 2017

tofumatt Jun 21, 2017

muffinresearch Jun 21, 2017 •

edited

Loading

tofumatt Jun 21, 2017

muffinresearch Jun 22, 2017

tofumatt Jun 22, 2017

muffinresearch Jun 21, 2017

kumar303 left a comment

kumar303 Jun 22, 2017

tofumatt Jun 22, 2017

kumar303 Jun 22, 2017

kumar303 Jun 22, 2017

tofumatt Jun 22, 2017

tofumatt Jun 23, 2017

tofumatt Jun 23, 2017

kumar303 Jun 22, 2017

tofumatt Jun 22, 2017

kumar303 Jun 22, 2017

kumar303 Jun 22, 2017

kumar303 Jun 22, 2017

kumar303 Jun 22, 2017

tofumatt Jun 22, 2017

kumar303 Jun 22, 2017

tofumatt Jun 22, 2017

kumar303 Jun 27, 2017

kumar303 Jun 22, 2017

kumar303 Jun 22, 2017

tofumatt commented Jun 23, 2017

kumar303 left a comment



		export default compose(
		safeAsyncConnect([{ promise: loadSearchResultsIfNeeded }]),

Create a desktop search page #2618

Create a desktop search page #2618

Conversation

tofumatt commented Jun 21, 2017

Small

Medium

Large/Desktop

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

muffinresearch Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Portrait

Landscape

Landscape (w/o content loaded)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Jun 23, 2017

kumar303 left a comment

Choose a reason for hiding this comment

muffinresearch Jun 21, 2017 •

edited

Loading