Search page tech improvement. Uses Pulse API to get search result. #348

mmmavis · 2017-02-04T00:17:42Z

Fixes #328

…ch result

mmmavis · 2017-02-04T00:18:57Z

@alanmoo since you've done similar things on the Science site, can you review this PR? 🍻

mmmavis · 2017-02-04T00:25:36Z

components/project-list/project-list.jsx

@@ -66,15 +88,6 @@ export default React.createClass({
      return entries.sort((a,b) => {
        return bookmarkedIdArray.indexOf(a.id.toString()) > bookmarkedIdArray.indexOf(b.id.toString());


Is it worth doing this sorting on Pulse API side instead?

e.g, /entries/?ids=3,1,5 will return entry no.3 first, followed by no.1, and lastly no.5.

Is this something you're using in a lot of places? The db returns these in numerical order by default, unfortunately so it's non-trivial to make it behave like this. That's not to say it's incredibly difficult, I'm just not sure it's worth the lift here if there's a JS function that does what you need relatively easily.

If that's the case don't worry about it. We just need to do the sorting once - for the initial /fav (bookmark) page render.

alanmoo · 2017-02-06T20:19:26Z

pages/search/search.jsx

-    if (event.keyCode === 27) { // escape key
-      this.clearSearch();
-    }
+  handleInputChange() {


If this doesn't do anything else, can you just call updateSearchQuery from the Debounce input itself?

alanmoo · 2017-02-06T20:35:13Z

components/project-list/project-list.jsx

  componentDidMount() {
    this.fetchData(this.props.params);
  },
  componentWillReceiveProps(nextProps) {
+    // Reset this.state.entries
+    this.resetEntries();


I don't think this should be called here. If I type in something new, it shouldn't clear the results while it's doing the search to find the new set; Doing that makes it feel broken because you see a moment of "0 results found", which isn't true. It's especially confusing if you're refining your search and the results you had disappear, and then the same ones reappear.

mmmavis · 2017-02-07T03:52:22Z

components/project-list/project-list.jsx

  componentDidMount() {
    this.fetchData(this.props.params);
  },
  componentWillReceiveProps(nextProps) {
+    // Reset state
+    this.resetState();


We should reset state as soon as a new search has been initiated. However, instead of showing "0 results found" I've update that message to Searching for entries that match ‘some_keyword’... (see render() below)

But most of the time you won't even have a chance to read that text if the connection is reasonably fast- this process results in a "flicker" of an intermediate state, instead of just changing from one set of results to another. If you want something in the middle, might I suggest a spinner or something to indicate something's happening in the background, without immediately clearing the results?

Think of the case where someone searches for "web" and then decides to add "literacy" to the end of it- it doesn't make sense that the results disappear before reappearing- they should just reduce to entries with the complete string "web literacy" in them.

Since we are doing search-as-you-type (we initiate the search when there's no activity happening in the input box for x amount of time), for me having the previous search result stayed while new search is happening in the background can confuse me. If there's no UI change go along with it, as an user I won't know a new search has already been initiated. Having a spinner instead of text Searching for entries that match ‘some_keyword’... sounds great! But I do think we should clear previous search result while fetching results from Pulse API for the current search keyword.

@alanmoo @kristinashu thoughts? This is what has been implemented in this PR

(note that there are some glitches from the GIF recorder in the last part, where some UI elements seem to collapse together)

But imagine the case where a user is typing just slow enough to trigger multiple searches- they'll have content appearing and disappearing seemingly at random.

I don't see the problem. I don't think we should reset the state as soon as a user start to type (it should continue to show results for "web" as you add "literacy"). It's ok to have a slight delay. Also don't think we even need a message or a spinner. Message is definitely too quick to read, so maybe a spinner but let's try having nothing and see if users complain.

But imagine the case where a user is typing just slow enough to trigger multiple searches- they'll have content appearing and disappearing seemingly at random.

Um I guess that's the nature of search-as-you-type. We don't start new search on every keystroke so to me that's fair enough. For non slower typers having previous search result stayed can make it look like the system is having problem detecting the new keyword user just typed, especially when network connection is bad.

Anyways, since you two both agree on the same approach I'll update my PR 😄

That's why I suggest a spinner or something to indicate that it's doing something, just not a message you have to read with disappearing content.

Spinners are nice.

I haven't read this whole thread, but if we get stuck on this, I suggest we consider adding a Go! button. Find as you type is only more awesome if it feels more awesome.

@alanmoo

That's why I suggest a spinner or something to indicate that it's doing something, just not a message you have to read with disappearing content.

I'm a bit confused about this part. Did you mean in the transition state (user searched for 'web' first and then 'web literacy') we show

input box: [ web literacy ] spinner icon [ matching result #1 for 'web' ] [ result #2 ] [ result #3 ] [ matching result #4 for 'web' ] [ result #6 ] [ result #6 ] ... ...

Yeah, pretty much. Just put the spinner somewhere that it's not "moving" the content when it appears/disappears. Though if you reduce the debounce time (maybe around 250ms), the whole thing should happen fast enough on reasonable network connections that the spinner doesn't show up for very long at all.

mmmavis · 2017-02-07T03:55:24Z

components/project-list/project-list.jsx

+        searchResult = (<p>{projects.length} {projects.length > 1 ? `results` : `result`} found for ‘{this.props.params.search}’</p>);
+      } else {
+        // We are still waiting to hear back from Pulse API. Let's show some 'searching' notice in the meantime.
+        searchResult = (<p>Searching for entries that match ‘{this.props.params.search}’...</p>);


@kristinashu @alanmoo
Searching for entries that match ‘some_keyword’... <= plz help fixing this sentence that I came up with 😂

How about just "Searching for..."

mmmavis · 2017-02-09T06:27:40Z

@alanmoo PR updated. Didn't add spinner but we can always add one later if needed.

alanmoo · 2017-02-09T22:38:09Z

I spun this up and accidentally (but in a reproducible way) got it into an inconsistent state. I clicked search, then typed w e space backspace b.

Looking at my network activity, the search for web was triggered and responded to (it has 73 items on staging), but this was never reflected in the UI count. When trying to reproduce it, I sometimes see the status flicker from 73 results found for web to 119 results found for we . It seems then like it's a race condition, where the web search returns first, but then the we search returns and takes priority. I suspect the issue is in the promise starting at line 68 of search.jsx. The result with more pages is taking longer to fetch, so it's setting state last.

mmmavis · 2017-02-10T00:52:57Z

i think i know what went wrong, will update!

…ted to the current search keyword

mmmavis · 2017-02-10T02:23:30Z

this should do... (hopefully)

alanmoo

This works, I'm just a bit thrown off by the logic- some things feel like they're still a bit fragile. Can you clarify some of the inline questions?

alanmoo · 2017-02-10T16:08:16Z

pages/search/search.jsx

+        <div className="project-list">
+          { searchResult }
+          { projects ? <div className="projects">{projects}</div> : null }
+          { showViewMoreBtn ? <div className="view-more"><button type="button" className="btn" onClick={this.handleViewMoreClick}>View more</button></div> : null }


This isn't hooked up

nice catch!

alanmoo · 2017-02-10T16:09:51Z

pages/search/search.jsx


+        if (data.next) {
+          // there are more matched entries in the database we need to fetch
+          this.fetchData(params, entries, apiPageIndex+1);


Why do we need to fetch all the pages up front if we've got a load more button later on?

@alanmoo

I didn't realize the count property returned from the API is the total count of all matched entries, not just how many entries are in that particular page. I thought I had to add them all up to get the total of matched entries count. Can we update the property name to something not so generic? Or at least have some docs for it?

Not sure how big our db will grow but since we only have ~200 entries now, making one fetch is enough to retrieve all matched entries (page_size is set to 996, the closest number to 1000 that's divisible by 6). Currently every time "Load more" is clicked we display 24 more entries. Users see those entries immediately after they click the button. We could change it so that we don't fetch all data at once when we load the page, we make new Pulse API call every time users click on "Load more". This means users will have to wait till we hear back from the call. If we decide to make this change I suggest bumping up " the number of entries in each batch" from 24 to a bigger number. Otherwise it can be quite annoying as users have to go through "click and wait" cycle for many times just to load a few hundreds of entries. /cc @kristinashu @xmatthewx I would like to know your thoughts on this 😉

If we decide to make this change I suggest bumping up " the number of entries in each batch" from 24 to a bigger number.

👍 as long as we don't overwhelm mobile users, loading 48 or more is fine by me. we could also pre-fetch as user scrolls down and nears the bottom.

With page_size of 996, are we fetching that many results or searching that many projects in total? In 2017, we might cross 1000 entries to search. We would need to lift that limit b/c pace will increase as people anticipate new network site. I wouldn't, however, expect to cross 1000 search results this year (for meaningful searches), so a cap on results is fine.

@mmmavis a lot of paginating APIs use count for the total count, with page and per_page for you to figure out exactly which set in [0...count] you're looking at, so I'd recommend we keep it like that.

As for #2, I'd also still recommend keeping the batches small: if someone's on 3G or worse, getting 200+ entries will still take much longer than is necessary for a user to see a first set of on-page entries loaded in the app.

@mmmavis

This is standard API behavior and the docs at the top of the entries endpoint already document the page_size, so it doesn't make sense to change this

?page_size= - Number of results on a page. Defaults to 48

Yeah I guess I question the purpose of the "Load more" button at all. Only thing I can think of is to avoid forcing the browser to load all those images unless they're wanted. Maybe we do fetch them all at once, and the "load more" button is a gate-keeper which then opens up to show the rest of the results at once? Presumably

alanmoo · 2017-02-10T16:31:53Z

pages/search/search.jsx

+      // then use searchQueryInUrl as the search param to fetch data from Pulse API
+      this.refs.searchInput.setState({value: searchQueryInUrl ? decodeURIComponent(searchQueryInUrl) : ``});
+      this.setState({
+        keywordBeingSearched: searchQueryInUrl


What's the purpose of this state variable? It seems like it's only checked on line 85, which could just read the value of the input box directly, no?

oh yea that's better, will update.

alanmoo · 2017-02-10T16:34:40Z

pages/search/search.jsx

+    let searchResult;
+
+    if (this.state.keywordSearched) {
+      let numEntriesMatched = this.state.entriesMatched.length;


If we don't fetch all the entries at the start, we can just use the count property on the API response here

alanmoo · 2017-02-10T16:55:19Z

components/project-list/project-list.jsx


-      // fetch data based on the new params props
+    if ( newIssueEntered ) {


Why issues specifically? And where does the issue prop actually get used? This makes me feel like something's not wired up to work best with React. Do you need a higher level component to fetch the data and then pass the data set down to the project list?

from routes.jsx

<Route path="issues"> <IndexRoute component={Issues} /> <Route path=":issue" component={Issue} /> </Route>

And my inline comments in project-list.jsx

// <ProjectList> doesn't always get unmounted and remounted when navigating between pages. // (e.g., When navigating between the /issue/issue-name pages <Issue><ProjectList></Issue> // do not get re-mounted since the same components are being rendered.) // It is treated as passing new props to <ProjectList {...newProps} />

And issue prop is being used in the next line after the one we are commenting on here

if ( newIssueEntered ) { // Fetch data based on the new params props to ensure data gets fetched and displayed accordingly. this.fetchData(nextProps.params); }

^ is the reasoning why I implemented it that way. Better solution is welcome!

oh yea but we should tackle it in a separate ticket/PR.

Ah, I see what's going on, but agreed it should be a separate ticket. I'll file it.

alanmoo

This is fine to merge, I left a few nitpicks that are related to the other ticket I'm going to file a follow up issue for, so they can be addressed there too.

alanmoo · 2017-02-10T22:07:23Z

scss/main.scss

+    margin-right: -$project-card-side-margin;
+  }
+
+  .view-more {


Can you use a .text-center class on the element instead?

alanmoo · 2017-02-10T22:17:00Z

scss/main.scss

+// project-list
+
+.project-list {
+  .projects {


If you use row flex-items-center as the classes instead, it does the same thing (with a minor difference in margin), and then you don't need this CSS. (By consistently using the bootstrap row/column classes, you avoid the margin issue.)

cool will file followup PR

alanmoo · 2017-02-10T22:26:15Z

Refactor suggestions detailed in #361

mmmavis · 2017-02-10T22:43:21Z

@alanmoo thanks I'll tackle #361 soon. Just wanna get this one in place first 😂

and @alanmoo I can't find our comments around "load more" button behaviour anymore... where did it go?!?!?

(dislike GitHub's new review interface)

Fixes #328 - search page tech improvement. Uses Pulse API to get sear…

12282c8

…ch result

mmmavis requested a review from alanmoo February 4, 2017 00:19

mmmavis commented Feb 4, 2017

View reviewed changes

alanmoo suggested changes Feb 6, 2017

View reviewed changes

mmmavis mentioned this pull request Feb 7, 2017

Added Google Analytics #349

Merged

13 tasks

show 'searching' notice

2d32687

mmmavis commented Feb 7, 2017

View reviewed changes

mmmavis added 3 commits February 8, 2017 22:18

Refine how search page fetches/displays data

dd36467

lower debounceTimeout value

5074ebc

moved .project-list css to main.scss since it's used by diff components

4b913e6

fixed confusing XHRs racing issue by ignoring response that is unrela…

21db773

…ted to the current search keyword

alanmoo reviewed Feb 10, 2017

View reviewed changes

mmmavis added 2 commits February 10, 2017 11:35

removed 'keywordBeingSearched' state

7d420f9

added missing handleViewMoreClick() method

f29cd96

mmmavis mentioned this pull request Feb 10, 2017

Build "add new entry" form #309

Closed

alanmoo approved these changes Feb 10, 2017

View reviewed changes

mmmavis merged commit 0627a2d into react Feb 10, 2017

mmmavis mentioned this pull request Feb 10, 2017

'load more' button CSS improvement #362

Merged

mmmavis deleted the issue-328-search-page-tech branch February 15, 2017 22:05

		@@ -66,15 +88,6 @@ export default React.createClass({
		return entries.sort((a,b) => {
		return bookmarkedIdArray.indexOf(a.id.toString()) > bookmarkedIdArray.indexOf(b.id.toString());


		// fetch data based on the new params props
		if ( newIssueEntered ) {

Search page tech improvement. Uses Pulse API to get search result. #348

Search page tech improvement. Uses Pulse API to get search result. #348

Conversation

mmmavis commented Feb 4, 2017

mmmavis commented Feb 4, 2017

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

mmmavis Feb 7, 2017 • 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

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

mmmavis commented Feb 9, 2017

alanmoo commented Feb 9, 2017 • edited Loading

mmmavis commented Feb 10, 2017

mmmavis commented Feb 10, 2017

alanmoo 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

mmmavis Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pomax Feb 10, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmmavis Feb 10, 2017 • 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

alanmoo 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

alanmoo commented Feb 10, 2017

mmmavis commented Feb 10, 2017

mmmavis Feb 7, 2017 •

edited

Loading

alanmoo commented Feb 9, 2017 •

edited

Loading

mmmavis Feb 10, 2017 •

edited

Loading

Pomax Feb 10, 2017 •

edited

Loading

mmmavis Feb 10, 2017 •

edited

Loading