Closed Bug 1378759 Opened 6 years ago Closed 6 years ago

Add Search Inputs to about:telemetry subsections

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1378759 - Improve the readability of table in about:telemetry
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1378759 - Add a search input to each section
59 bytes, text/x-review-board-request
chutten
: review+
Details 
* Add border to table
* Use a smaller font
* Reduce the sidebar size
* More...
Redesign which section? All sections?
Comment on attachment 8883931 [details]
Bug 1378759 - Add a search input to each section

https://reviewboard.mozilla.org/r/154910/#review159914

::: commit-message-f509c:1
(Diff revision 1)
> +Bug 1378759 - Redesign the section in about:telemetry r?chutten

This is more about redesigning tables than a particular (unnamed) section. Where do we use tables that this would be visible? What does this improve? Readability? General look-and-feel?
Attachment #8883931 - Flags: review?(chutten) → review-
Comment on attachment 8883931 [details]
Bug 1378759 - Add a search input to each section

https://reviewboard.mozilla.org/r/154910/#review160040
Attachment #8883931 - Flags: review?(chutten) → review+
Attachment #8883931 - Attachment is obsolete: true
Comment on attachment 8885501 [details]
Bug 1378759 - Improve the readability of table in about:telemetry

https://reviewboard.mozilla.org/r/156372/#review161616
Attachment #8885501 - Flags: review?(chutten) → review+
Comment on attachment 8885502 [details]
Bug 1378759 - Add a search input to each section

https://reviewboard.mozilla.org/r/156374/#review161618

Just a few things.

::: toolkit/content/aboutTelemetry.js:1417
(Diff revision 1)
> -    this.idleTimeout = setTimeout( () => {
> -      Histogram.filterHistograms(document.getElementById(this.getAttribute("target_id")), this.value);
> -    }, FILTER_IDLE_TIMEOUT);
> +  search(text) {
> +    let selectedSection = document.querySelector(".data-section.active");
> +    if (selectedSection.id.includes("histograms")) {
> +      let histograms = selectedSection.getElementsByClassName("histogram");
> +      this.filterElements(histograms, text);
> +    } else if (selectedSection.id === "keyed-histograms-section") {

This condition will never be hit as "keyed-histograms-section".includes("histograms")

::: toolkit/content/aboutTelemetry.js:1418
(Diff revision 1)
> -      Histogram.filterHistograms(document.getElementById(this.getAttribute("target_id")), this.value);
> -    }, FILTER_IDLE_TIMEOUT);
> +    let selectedSection = document.querySelector(".data-section.active");
> +    if (selectedSection.id.includes("histograms")) {
> +      let histograms = selectedSection.getElementsByClassName("histogram");
> +      this.filterElements(histograms, text);
> +    } else if (selectedSection.id === "keyed-histograms-section") {
> +      // let keyedHistograms = selectedSection.getElementsByClassName("keyed-histogram");

Checking in commented-out code.

::: toolkit/content/aboutTelemetry.js:1427
(Diff revision 1)
> +      for (let table of tables) {
> +        this.filterElements(table.rows, text);
> +      }
> +    }
> +  }
> -  }
> +}

Semicolon needed? What does lint say?

::: toolkit/content/aboutTelemetry.js:1825
(Diff revision 1)
> +function displaySearch(selectedSection) {
> +  let blacklist = [
> +    "home",
> +  ];
> +  let search = document.getElementById("search");
> +  search.hidden = blacklist.includes(selectedSection);

Ah, so this is basically "// TODO: Implement global search for the Home section". Got it.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:37
(Diff revision 1)
>  telemetryLogHeadingTimestamp = Timestamp
>  
>  telemetryLogHeadingData = Data
>  
> +# Note to translators:
> +# - %1$S will be replaced by the section name

Should let translators know where the section names come from. If they're English from the structure of the ping, it'd be good to explain that (maybe link to the readthedocs page detailing ping structure for their reference)
Attachment #8885502 - Flags: review?(chutten) → review-
Comment on attachment 8885502 [details]
Bug 1378759 - Add a search input to each section

https://reviewboard.mozilla.org/r/156374/#review161618

> Semicolon needed? What does lint say?

Lint does not complain here, but I added a semicolon.
Comment on attachment 8885502 [details]
Bug 1378759 - Add a search input to each section

https://reviewboard.mozilla.org/r/156374/#review162150

::: toolkit/content/aboutTelemetry.js:1400
(Diff revisions 1 - 5)
>      }
> +    return {filter, isPassFunc}
> +  },
> +
> +  filterElements(elements, filterText) {
> +    let res = this.chooseFilter(filterText);

You can use destructuring assignment (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment) to unpack these in a single line.
Attachment #8885502 - Flags: review?(chutten) → review-
Summary: Redesign the section in about:telemetry → Add Search Inputs to about:telemetry subsections
Comment on attachment 8885502 [details]
Bug 1378759 - Add a search input to each section

https://reviewboard.mozilla.org/r/156374/#review163110

Looks good to me
Attachment #8885502 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05c454cb13f3
Improve the readability of table in about:telemetry r=chutten
https://hg.mozilla.org/integration/autoland/rev/25c4b4213127
Add a search input to each section r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/05c454cb13f3
https://hg.mozilla.org/mozilla-central/rev/25c4b4213127
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.