basic search functionality from probe search service #602

openjck · 2020-06-10T21:02:22Z

Supersedes #594 to merge into the main branch.

src/components/search/Search.svelte

robhudson · 2020-06-11T17:51:25Z

src/state/api.js

+    params.set('limit', resultsLimit);
+    params.set('select', 'name,definition,type');
+    params.set('product', `eq.${product}`);
+    params.set('type', 'neq.event');


The type may depend on product. Although I don't think this hurts fenix, or other glean products currently.

Yep I expect adjustments here - should be easier now that it's not a long string.

I think we'll probably understand more when we start pointing this at Fenix probes in GLAM, but let's expect these to be moved to firefox-desktop.js at some point

src/state/api.js

robhudson · 2020-06-11T17:55:49Z

src/components/search/Search.svelte

+        if (aHas && !bHas) return -1;
+        if (bHas && !aHas) return 1;
+        return 0;
+      });


In the near future I plan to update the probe-search to rank the results based on weighting of the matches. Once that lands this could likely go away. Just a heads up.

Yea I'd prefer if this were gone.

hamilton

This is muuuuch nicer already! I have a few more changes / comments to request. Overall very much on the right track. I'm mostly focused on meeting (mostly) the same design as before, but there might be a few tweaks to make sure we're at implicit parity here.

hamilton · 2020-06-11T19:37:17Z

src/state/api.js

@@ -54,3 +54,32 @@ export async function getProbeData(params, token) {
  });
  return data;
 }
+
+export async function getSearchResults(queryString, resultsLimit) {


Since you're returning a Promise here (and the await keyword below is not necessary) we can just make this a regular function

Suggested change

export async function getSearchResults(queryString, resultsLimit) {

export function getSearchResults(queryString, resultsLimit) {

hamilton · 2020-06-11T19:38:08Z

src/state/api.js

+    return URLResult;
+  };
+
+  return await fetch(getFormattedSearchURL(queryString)).then(async (r) => {


I don't think the await keyword is necessary here, since I would presume that whatever utilizes the return value of this function will be awaiting anyway

Suggested change

return await fetch(getFormattedSearchURL(queryString)).then(async (r) => {

return fetch(getFormattedSearchURL(queryString)).then(async (r) => {

hamilton · 2020-06-11T19:41:16Z

src/components/search/SearchResults.svelte

+              <div>your search produced 0 results</div>
+            </div>
+          {:else if results.status}
+            <!-- TODO: This should be pretty. -->


Suggested change

hamilton · 2020-06-11T19:43:40Z

src/components/search/SearchResults.svelte

+      transition:fly={{ duration: 20, y: -10 }}
+      class="telemetry-results">
+        <div class="header-container">
+          {#if results.length}


I think I mentioned this in the previous PR, but it feels like there should be another case here, where if we are either waiting for the throttle timer to finish or we're waiting for a result set, where we say fetching probes + a spinner or something like that. Right now, I type and nothing immediately happens, and it feels like a step back in terms of experience.

hamilton · 2020-06-11T19:46:37Z

src/components/search/SearchResults.svelte

+          {:else if results.status}
+            <!-- TODO: This should be pretty. -->
+            <div class="header" out:fly={{ x: 5, duration: 200 }}>
+              <div>search failed with status {results.status}</div>


Suggested change

<div>search failed with status {results.status}</div>

<div>hmm ... having trouble reaching the search service ({results.status}). Reach out on #glam if this problem persists</div>

openjck · 2020-06-11T22:56:44Z

package.json

@@ -54,6 +54,7 @@
    "d3-time-format": "2.2.3",
    "flexsearch": "0.6.30",
    "page": "1.11.6",
+    "throttle-debounce": "^2.2.1",


Please pin the dependency by running npm install -E throttle-debounce.

More information on why we've been pinning dependencies:

https://docs.renovatebot.com/dependency-pinning/

openjck · 2020-06-11T23:05:27Z

rollup.config.js

@@ -9,6 +9,15 @@ import postcss from 'rollup-plugin-postcss';

 const production = process.env.NODE_ENV === 'production';

+// We can provide this as another ENV var if desired (uses NODE_ENV currently).


Should we decide and remove this comment before merging?

openjck · 2020-06-11T23:17:55Z

This brings our ESLint problem count down by 12! 🎉

edit: Wait, I got that backwards. Sorry! 😆

openjck · 2020-06-11T23:22:44Z

src/routing/Router.svelte

@@ -54,7 +54,7 @@
      if (probeName) {
        store.setField('probeName', probeName);
        if ($probeSet) {
-          let newProbe = $probeSet.find((p) => p.name === probeName);
+          let newProbe = $probeSet.find((p) => p.name.toLowerCase() === probeName.toLowerCase());


Do we need to compare against probeName.toLowerCase()? We don't support non-lowercase probe names in the URL and to avoid canonicality issues I don't think we should.

http://localhost:5000/firefox/probe/gC_mS/explore?activeBuckets=%5B%22no%22%2C%22yes%22%5D&aggregationLevel=build_id&channel=nightly&os=Windows&process=content&proportionMetricType=proportions&timeHorizon=MONTH&visiblePercentiles=%5B95%2C75%2C50%2C25%2C5%5D

openjck · 2020-06-11T23:29:13Z

src/components/search/Search.svelte

-    inputElement.blur();
-    store.setField('searchIsActive', false);
+  inputElement.blur();
+  searchIsActive = false;
 }

 async function onKeypress(event) {


Small thing, but since we only need the key from the event, we could save one line by doing:

async function onKeypress({ key }) {

Personal preference one way or the other. Not a big deal.

openjck · 2020-06-11T23:31:32Z

src/components/search/Search.svelte

 import { fly } from 'svelte/transition';
+import { cubicOut } from 'svelte/easing';


4:10 error 'cubicOut' is defined but never used no-unused-vars

openjck · 2020-06-11T23:32:08Z

src/components/search/Search.svelte

 import { fly } from 'svelte/transition';
+import { cubicOut } from 'svelte/easing';
 import SearchIcon from 'udgl/icons/Search.svelte';
 import LineSegSpinner from 'udgl/LineSegSpinner.svelte';


Due to changes below:

6:8 error 'LineSegSpinner' is defined but never used no-unused-vars

openjck · 2020-06-11T23:32:26Z

src/components/search/Search.svelte

 }

+let searchResults = Promise.resolve([]);


41:5 error 'searchResults' is assigned a value but never used no-unused-vars

openjck · 2020-06-11T23:32:37Z

src/components/search/Search.svelte

+let searchResults = Promise.resolve([]);
+let query = '';
+
+const handleSearchInput = throttle(SEARCH_THROTTLE_TIME, ({target: {value}}) => {


44:59 error A space is required after '{' object-curly-spacing 44:68 error A space is required after '{' object-curly-spacing 44:74 error A space is required before '}' object-curly-spacing 44:75 error A space is required before '}' object-curly-spacing

propaganda I say - will fix

openjck · 2020-06-11T23:33:01Z

src/components/search/SearchResults.svelte

@@ -7,12 +7,15 @@ import { afterUpdate } from 'svelte';
 import Portal from 'udgl/Portal.svelte';
 import LineSegSpinner from 'udgl/LineSegSpinner.svelte';


Due to changes below:

8:8 error 'LineSegSpinner' is defined but never used no-unused-vars

openjck · 2020-06-11T23:33:48Z

src/components/search/SearchResults.svelte

-          {#if $searchResults.total}
-          <div class="header header--loaded" in:fly={{ x: -5, duration: 200 }}>
-              <div>found {$searchResults.results.length} of
-                  {formatTotal($searchResults.total)} probes


This removal raises the following error on line 25. (GitHub won't allow me to comment on line 25.)

25:5 error 'formatTotal' is assigned a value but never used no-unused-vars

openjck · 2020-06-11T23:34:13Z

src/components/search/SearchResults.svelte

    }
    if (key === 'Escape') {
-      store.setField('searchIsActive', true);
+      seachIsActive = true;


61:7 error 'seachIsActive' is not defined no-undef

openjck · 2020-06-11T23:50:05Z

src/components/search/Search.svelte

+const handleSearchInput = throttle(SEARCH_THROTTLE_TIME, ({target: {value}}) => {
+  query = value;
+  searchResults = getSearchResults(query, SEARCH_RESULTS_LIMIT).then((r) => {
+    // sort these?


Are they already being sorted or is this a reference to something else?

We're sorting them here though it's not the right approach. Plan is for PSS to give us weighted results (coming soon) so that searches found in name weigh more heavily than description/type.

openjck · 2020-06-11T23:55:23Z

src/components/search/Search.svelte

@@ -62,15 +85,14 @@ async function onKeypress(event) {
    align-items: stretch;
    background-color: var(--input-background-color);
    border-radius: var(--space-1h);
-
+    position: relative;


Is this needed? I don't see a difference but I may be missing something.

It was with the old error notice but not anymore. Good find.

openjck · 2020-06-24T03:13:25Z

Are we still using the flexsearch package on this branch?

robhudson · 2020-06-25T18:58:57Z

CHANGELOG.md

@@ -4,6 +4,8 @@

 - Fix issue with categorical explorer counts
  ([#587](https://github.com/mozilla/glam/pull/587))
+- Implement Probe Search Service


Implement sounds like this PR created the search service instead of updating the search to use the search service.

openjck mentioned this pull request Jun 10, 2020

basic search functionality from probe search service #594

Closed

openjck requested review from hamilton and robhudson June 10, 2020 21:02

robhudson reviewed Jun 10, 2020

View reviewed changes

src/components/search/Search.svelte Outdated Show resolved Hide resolved

spasovski force-pushed the pss-phase-1 branch from 8fc73f7 to b5601cd Compare June 11, 2020 17:45

robhudson reviewed Jun 11, 2020

View reviewed changes

src/state/api.js Outdated Show resolved Hide resolved

spasovski force-pushed the pss-phase-1 branch from b5601cd to 64b8eb5 Compare June 11, 2020 17:54

robhudson reviewed Jun 11, 2020

View reviewed changes

spasovski force-pushed the pss-phase-1 branch from 64b8eb5 to f804708 Compare June 11, 2020 18:15

hamilton mentioned this pull request Jun 11, 2020

basic search functionality from probe search service #611

Closed

4 tasks

hamilton suggested changes Jun 11, 2020

View reviewed changes

openjck commented Jun 11, 2020

View reviewed changes

spasovski force-pushed the pss-phase-1 branch 2 times, most recently from 7109e6a to 8488979 Compare June 12, 2020 00:28

hamilton self-requested a review June 17, 2020 15:02

spasovski force-pushed the pss-phase-1 branch from 8488979 to d2751c9 Compare June 23, 2020 18:15

spasovski force-pushed the pss-phase-1 branch from d2751c9 to a4ab58b Compare June 23, 2020 19:11

robhudson reviewed Jun 25, 2020

View reviewed changes

basic search functionality from probe search service

e2dc453

spasovski force-pushed the pss-phase-1 branch from a4ab58b to e2dc453 Compare June 25, 2020 19:14

spasovski merged commit 3b69309 into main Jun 29, 2020

cmharlow deleted the pss-phase-1 branch March 18, 2022 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic search functionality from probe search service #602

basic search functionality from probe search service #602

openjck commented Jun 10, 2020

robhudson Jun 11, 2020

spasovski Jun 11, 2020

hamilton Jun 11, 2020

robhudson Jun 11, 2020

spasovski Jun 11, 2020

hamilton left a comment

hamilton Jun 11, 2020

hamilton Jun 11, 2020

openjck Jun 11, 2020

hamilton Jun 11, 2020

hamilton Jun 11, 2020

hamilton Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck commented Jun 11, 2020 •

edited

Loading

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

spasovski Jun 12, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

openjck Jun 11, 2020

spasovski Jun 12, 2020

openjck Jun 11, 2020

spasovski Jun 12, 2020

openjck commented Jun 24, 2020

robhudson Jun 25, 2020

	export async function getSearchResults(queryString, resultsLimit) {
	export function getSearchResults(queryString, resultsLimit) {

	return await fetch(getFormattedSearchURL(queryString)).then(async (r) => {
	return fetch(getFormattedSearchURL(queryString)).then(async (r) => {

	<!-- TODO: This should be pretty. -->
	<!-- FIXME: This should be pretty. -->

	<div>search failed with status {results.status}</div>
	<div>hmm ... having trouble reaching the search service ({results.status}). Reach out on #glam if this problem persists</div>

		@@ -9,6 +9,15 @@ import postcss from 'rollup-plugin-postcss';

		const production = process.env.NODE_ENV === 'production';

		// We can provide this as another ENV var if desired (uses NODE_ENV currently).

		import { fly } from 'svelte/transition';
		import { cubicOut } from 'svelte/easing';

		@@ -7,12 +7,15 @@ import { afterUpdate } from 'svelte';
		import Portal from 'udgl/Portal.svelte';
		import LineSegSpinner from 'udgl/LineSegSpinner.svelte';

basic search functionality from probe search service #602

basic search functionality from probe search service #602

Conversation

openjck commented Jun 10, 2020

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

hamilton 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

openjck commented Jun 11, 2020 • 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

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

openjck commented Jun 24, 2020

Choose a reason for hiding this comment

openjck commented Jun 11, 2020 •

edited

Loading