about:telemetry - Clicking on a subsection then clicking on a section doesn't unfilter

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: chutten, Assigned: flyingrub)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

2 years ago
1) Click on, say, Environment Data
  - shows all environment data
2) Click on Build
  - shows build, environment data
3) Click on Environment Data

Expected: shows all environment data
Actual: still shows just the build environment data


I think the other subsections' data sections need to toggle back to visible when selecting the section in the sidebar.
Blocks: 1384534
No longer depends on: 1384534
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Priority: P2 → P1

Comment 2

2 years ago
mozreview-review
Comment on attachment 8895137 [details]
Bug 1385270 - Fix about:telemetry subsection display

https://reviewboard.mozilla.org/r/166262/#review172308

I tested this locally with the steps from the comment 0 and the behavior wasn't fixed.
Attachment #8895137 - Flags: review?(gfritzsche) → review-
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8895137 [details]
Bug 1385270 - Fix about:telemetry subsection display

https://reviewboard.mozilla.org/r/166262/#review173558

This fixes the problem for me, but i have some small issues below.
Also, this needs rebasing - or does it depend on some other patch?

::: toolkit/content/aboutTelemetry.js:1896
(Diff revision 2)
>   */
>  function show(selected) {
>    let current_button = document.querySelector(".category.selected");
>    current_button.classList.remove("selected");
> +  if (current_button.classList.contains("has-subsection")) {
> +    Array.from(current_button.children).forEach((subsection) =>

Why not just use a normal loop?
```
for (let subsection of current_button.children) {
 ...
```

::: toolkit/content/aboutTelemetry.js:1897
(Diff revision 2)
>  function show(selected) {
>    let current_button = document.querySelector(".category.selected");
>    current_button.classList.remove("selected");
> +  if (current_button.classList.contains("has-subsection")) {
> +    Array.from(current_button.children).forEach((subsection) =>
> +      subsection.classList.remove("selected")

Missing semicolon at the end of this line.

::: toolkit/content/aboutTelemetry.js:1909
(Diff revision 2)
>    let selectedValue = selected.getAttribute("value");
>    let current_section = document.querySelector("section.active");
>    let selected_section = document.getElementById(selectedValue);
> +  let subsections = current_section.querySelectorAll(".sub-section");
> +  if (subsections) {
> +    subsections.forEach((subsection) => subsection.hidden = false);

Same here, why not a normal loop?
Attachment #8895137 - Flags: review?(gfritzsche)
Assignee

Comment 5

2 years ago
mozreview-review
Comment on attachment 8895137 [details]
Bug 1385270 - Fix about:telemetry subsection display

https://reviewboard.mozilla.org/r/166262/#review173598

::: toolkit/content/aboutTelemetry.js:1896
(Diff revision 2)
>   */
>  function show(selected) {
>    let current_button = document.querySelector(".category.selected");
>    current_button.classList.remove("selected");
> +  if (current_button.classList.contains("has-subsection")) {
> +    Array.from(current_button.children).forEach((subsection) =>

I had the habit to use this way. I'm changing it back to simple for loop :).

::: toolkit/content/aboutTelemetry.js:1909
(Diff revision 2)
>    let selectedValue = selected.getAttribute("value");
>    let current_section = document.querySelector("section.active");
>    let selected_section = document.getElementById(selectedValue);
> +  let subsections = current_section.querySelectorAll(".sub-section");
> +  if (subsections) {
> +    subsections.forEach((subsection) => subsection.hidden = false);

It's more concise here to use forEach. I think we can keep it ?
Comment hidden (mozreview-request)
(In reply to flyingrub from comment #5)
> ::: toolkit/content/aboutTelemetry.js:1909
> (Diff revision 2)
> >    let selectedValue = selected.getAttribute("value");
> >    let current_section = document.querySelector("section.active");
> >    let selected_section = document.getElementById(selectedValue);
> > +  let subsections = current_section.querySelectorAll(".sub-section");
> > +  if (subsections) {
> > +    subsections.forEach((subsection) => subsection.hidden = false);
> 
> It's more concise here to use forEach. I think we can keep it ?

Readability and standard control structures trump saving two lines for me here.
Attachment #8895137 - Flags: review?(gfritzsche)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8895137 [details]
Bug 1385270 - Fix about:telemetry subsection display

https://reviewboard.mozilla.org/r/166262/#review173974
Attachment #8895137 - Flags: review?(gfritzsche) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/47cf76ef4d3d
Fix about:telemetry subsection display r=gfritzsche
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47cf76ef4d3d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter

Updated

2 years ago
Blocks: 1391647
You need to log in before you can comment on or make changes to this bug.