Closed Bug 1393478 Opened 7 years ago Closed 7 years ago

Improve the search in about:telemetry

Categories

(Toolkit :: Telemetry, defect, P1)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1393478 - Clear the search filter when switching section
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1393478 - Display when no results are available in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1393478 - Keep the search input at the same place in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1393478 - Fix subsection search in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
* Display a text message like "Nothing was found" when no data is available for the current filter. * Clear the search filter when switching section. * The search term could be included in the url for the ease of sharing.
Oh, and the global search on "Home" shouldn't have a placeholder of "Find in Home" it should have a placeholder more like "Find in entire ping" or just something bland and generic like "Search" The following two notes are from the feedback etherpad (https://public.etherpad-mozilla.org/p/about_telemetry_feedback): * search result should have headings for "scalars", "histograms", etc. (currently i can't tell what kind of measurement i found) * the search in start page is named "find in home", should be "find in all" or similar
Comment on attachment 8901202 [details] Bug 1393478 - Clear the search filter when switching section https://reviewboard.mozilla.org/r/172666/#review178556 ::: toolkit/content/aboutTelemetry.js (Diff revision 1) > let blacklist = [ > "raw-payload-section" > ]; > let search = document.getElementById("search"); > search.hidden = blacklist.includes(selectedSection); > - // Filter element on section change. Does this mean it will clear the filter when browsing through pings? Is this intentional?
Comment on attachment 8901202 [details] Bug 1393478 - Clear the search filter when switching section https://reviewboard.mozilla.org/r/172666/#review178556 > Does this mean it will clear the filter when browsing through pings? Is this intentional? this is fixed in the next commit :)
Comment on attachment 8902184 [details] Bug 1393478 - Display when no results are available in about:telemetry https://reviewboard.mozilla.org/r/173658/#review179012 ::: toolkit/content/aboutTelemetry.css:159 (Diff revision 1) > } > > +#no-search-results { > + position: fixed; > + top: 50%; > + left: 50%; Rather than cook your own, why not steal about:preferences' "Sorry! There are no results in <Section> for "<searchTerms>"" .... and its cute bear, too :D ::: tools/lint/hooks.py:18 (Diff revision 1) > > > def run_mozlint(hooktype, args): > # --quiet prevents warnings on eslint, it will be ignored by other linters > python = find_executable('python2.7') or find_executable('python') > - cmd = [python, os.path.join(topsrcdir, 'mach'), 'lint', '--quiet'] > + cmd = [python, os.path.join(topsrcdir, 'mach'), 'lint', '--quiet', '-wo'] I think this isn't something you meant to include
Attachment #8902184 - Flags: review?(chutten) → review-
Comment on attachment 8901202 [details] Bug 1393478 - Clear the search filter when switching section https://reviewboard.mozilla.org/r/172666/#review179016 r+ pending the fixing of the "lose search filter when switching pings" issue in the next commit.
Attachment #8901202 - Flags: review?(chutten) → review+
Comment on attachment 8902259 [details] Bug 1393478 - Keep the search input at the same place in about:telemetry https://reviewboard.mozilla.org/r/173788/#review179098 Consistency is good.
Attachment #8902259 - Flags: review?(chutten) → review+
Comment on attachment 8902260 [details] Bug 1393478 - Fix subsection search in about:telemetry https://reviewboard.mozilla.org/r/173790/#review179100 ::: toolkit/content/aboutTelemetry.js:1438 (Diff revision 2) > let datas = key.querySelector("table").rows; > keyedElements.push({key, datas}); > } > noSearchResults = this.filterKeyedElements(keyedElements, text); > + } else if (section.querySelector(".sub-section")) { > + let keyedElements = []; They're not really keyed, though, are they. They're subsections? I realize we're using filterKeyedElements to filter the subsections, but naming is important for how readable the code will be.
Attachment #8902260 - Flags: review?(chutten)
Comment on attachment 8902184 [details] Bug 1393478 - Display when no results are available in about:telemetry https://reviewboard.mozilla.org/r/173658/#review179096 ::: toolkit/content/aboutTelemetry.js:1417 (Diff revision 3) > let sectionId = document.querySelector(".category.selected").getAttribute("value"); > section = document.getElementById(sectionId); > } > + let noSearchResults; > if (section.id === "home-section") { > - this.homeSearch(text); > + return this.homeSearch(text); Why is searching on home a special case for search? Does that mean we don't get a "No Results" prompt if we search on the home page? ::: toolkit/content/aboutTelemetry.js:2121 (Diff revision 3) > > // Update ping data when async Telemetry init is finished. > Telemetry.asyncFetchTelemetryData(async () => { > - await PingPicker.update(); > urlStateRestore(); > + await PingPicker.update(); These changes seem unintentionally added
Attachment #8902184 - Flags: review?(chutten) → review-
Comment on attachment 8902286 [details] Bug 1393478 - Fix Search on home https://reviewboard.mozilla.org/r/173812/#review179164 This seems fine but putting fixes to bugs introduced by earlier commits in the same bug is confusing and not kind to the source history. Please rework the commits so that the fixes are squashed in the commits that introduced the problems.
Attachment #8902286 - Flags: review?(chutten) → review-
Comment on attachment 8902184 [details] Bug 1393478 - Display when no results are available in about:telemetry https://reviewboard.mozilla.org/r/173658/#review179258 ::: toolkit/content/aboutTelemetry.js:1417 (Diff revision 3) > let sectionId = document.querySelector(".category.selected").getAttribute("value"); > section = document.getElementById(sectionId); > } > + let noSearchResults; > if (section.id === "home-section") { > - this.homeSearch(text); > + return this.homeSearch(text); This is for a lint issue. The search on home is introduced on a next commit. ::: toolkit/content/aboutTelemetry.js:2121 (Diff revision 3) > > // Update ping data when async Telemetry init is finished. > Telemetry.asyncFetchTelemetryData(async () => { > - await PingPicker.update(); > urlStateRestore(); > + await PingPicker.update(); This is needed to keep the search state when reloading.
Attachment #8902286 - Attachment is obsolete: true
Comment on attachment 8902184 [details] Bug 1393478 - Display when no results are available in about:telemetry https://reviewboard.mozilla.org/r/173658/#review179498
Attachment #8902184 - Flags: review?(chutten) → review+
Comment on attachment 8902260 [details] Bug 1393478 - Fix subsection search in about:telemetry https://reviewboard.mozilla.org/r/173790/#review179500
Attachment #8902260 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/bed1aa8b73f5 Clear the search filter when switching section r=chutten https://hg.mozilla.org/integration/autoland/rev/a841cfcda18f Display when no results are available in about:telemetry r=chutten https://hg.mozilla.org/integration/autoland/rev/6f00c4602bc1 Keep the search input at the same place in about:telemetry r=chutten https://hg.mozilla.org/integration/autoland/rev/56940b743275 Fix subsection search in about:telemetry r=chutten
Keywords: checkin-needed
(In reply to Chris H-C :chutten from comment #1) > The following two notes are from the feedback etherpad > (https://public.etherpad-mozilla.org/p/about_telemetry_feedback): > > * search result should have headings for "scalars", "histograms", etc. > (currently i can't tell what kind of measurement i found) > * the search in start page is named "find in home", should be "find in all" > or similar Headings in search results sounds like bug 1395091, but the second one will need its own bug now, right?
Flags: needinfo?(flyinggrub)
This as already been fixed in Bug 1392230 :)
Flags: needinfo?(flyinggrub)
I don't see this fixed in the current Nightly build. I get some sub-level headings (e.g. "settings"), but i don't see any top-level headings (e.g. "scalars", "environment").
Flags: needinfo?(flyinggrub)
I was talking about the second point about the placeholder text of the search on the home page. :D The heading bug is not fixed for now.
Flags: needinfo?(flyinggrub)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: