[RPP] Make sidebar insight clickable to toggle. [devtools/devtools-frontend : main]

0 views
Skip to first unread message

Adriana Ixba (Gerrit)

unread,
Jun 26, 2024, 10:13:44 PM (3 days ago) Jun 26
to Alina Varkki, Nancy Li, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Alina Varkki

Adriana Ixba voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention is currently required from:
  • Alina Varkki
Submit Requirements:
  • requirement is not satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I758c279b0de7bd9ae2553a7d03aeed1d1af1221f
Gerrit-Change-Number: 5661588
Gerrit-PatchSet: 2
Gerrit-Owner: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Alina Varkki <alina...@chromium.org>
Gerrit-CC: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-CC: Nancy Li <nan...@chromium.org>
Gerrit-Attention: Alina Varkki <alina...@chromium.org>
Gerrit-Comment-Date: Thu, 27 Jun 2024 02:13:41 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
unsatisfied_requirement
open
diffy

Alina Varkki (Gerrit)

unread,
Jun 27, 2024, 10:50:43 AM (3 days ago) Jun 27
to Adriana Ixba, Nancy Li, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org
Attention needed from Adriana Ixba

Alina Varkki voted and added 5 comments

Votes added by Alina Varkki

Code-Review+1

5 comments

Patchset-level comments
File-level comment, Patchset 2 (Latest):
Alina Varkki . resolved

LGTM with a few suggestions 😊

File front_end/panels/timeline/components/Sidebar.ts
Line 170, Patchset 2 (Latest): this.#lcpPhasesToggled = false;
Alina Varkki . unresolved

Maybe `lcpPhasesExpanded`?

File front_end/panels/timeline/components/SidebarInsight.ts
Line 12, Patchset 2 (Latest): toggled: boolean;
Alina Varkki . unresolved

`expanded`

File front_end/panels/timeline/components/sidebarInsight.css
Line 19, Patchset 2 (Latest):.insight-closed {
Alina Varkki . unresolved

what do you think about making the closed view a bit darker on hover? to give it more of a button look

something like 

```
  &.closed:hover { 
    background-color: \\something a bit darker\\;
  }
```
Line 7, Patchset 2 (Latest):.insight {
display: block;
width: auto;
height: auto;
margin: 10px 0;
border: 1px solid var(--sys-color-outline);
padding: 10px;
border-radius: 3px;
overflow: hidden;
background-color: var(--sys-color-base);
}

.insight-closed {
display: block;
width: auto;
height: auto;
margin: 10px 0;
padding: 10px;
border-radius: 3px;
overflow: hidden;
background-color: var(--sys-color-neutral-container);
}
Alina Varkki . unresolved

Most of the css for `.insight` and `.insight-closed` is the same. We can use ampersand (&) selector to overwrite the few differences that apply to the `closed` element.


Something like this should work:

```suggestion
.insight {
  display: block;
  width: auto;
  height: auto;
  margin: 10px 0;
  padding: 10px;
  border-radius: 3px;
  overflow: hidden;
  border: 1px solid var(--sys-color-outline);
  background-color: var(--sys-color-base);
  &.closed { 
    background-color: var(--sys-color-neutral-container);
    border: none;
  }
}
```

And then on the `.closed` component it would be
`<div class="insight closed">`

Open in Gerrit

Related details

Attention is currently required from:
  • Adriana Ixba
Submit Requirements:
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I758c279b0de7bd9ae2553a7d03aeed1d1af1221f
Gerrit-Change-Number: 5661588
Gerrit-PatchSet: 2
Gerrit-Owner: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Alina Varkki <alina...@chromium.org>
Gerrit-CC: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-CC: Nancy Li <nan...@chromium.org>
Gerrit-Attention: Adriana Ixba <ai...@google.com>
Gerrit-Comment-Date: Thu, 27 Jun 2024 14:50:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
open
diffy

Adriana Ixba (Gerrit)

unread,
Jun 27, 2024, 2:29:50 PM (3 days ago) Jun 27
to Alina Varkki, Nancy Li, Devtools-frontend LUCI CQ, devtools-rev...@chromium.org

Adriana Ixba voted and added 5 comments

Votes added by Adriana Ixba

Commit-Queue+2

5 comments

Patchset-level comments
Alina Varkki . resolved

LGTM with a few suggestions 😊

Adriana Ixba

Thanks so much! :)

File front_end/panels/timeline/components/Sidebar.ts
Line 170, Patchset 2: this.#lcpPhasesToggled = false;
Alina Varkki . resolved

Maybe `lcpPhasesExpanded`?

Adriana Ixba

Done

File front_end/panels/timeline/components/SidebarInsight.ts
Line 12, Patchset 2: toggled: boolean;
Alina Varkki . resolved

`expanded`

Adriana Ixba

Done

File front_end/panels/timeline/components/sidebarInsight.css
Line 19, Patchset 2:.insight-closed {
Alina Varkki . resolved

what do you think about making the closed view a bit darker on hover? to give it more of a button look

something like 

```
  &.closed:hover { 
    background-color: \\something a bit darker\\;
  }
```
Adriana Ixba

yes, i like this! Done


display: block;
width: auto;
height: auto;
margin: 10px 0;
border: 1px solid var(--sys-color-outline);
padding: 10px;
border-radius: 3px;
overflow: hidden;
background-color: var(--sys-color-base);
}

.insight-closed {
display: block;
width: auto;
height: auto;
margin: 10px 0;
padding: 10px;
border-radius: 3px;
overflow: hidden;
background-color: var(--sys-color-neutral-container);
}
Alina Varkki . resolved

Most of the css for `.insight` and `.insight-closed` is the same. We can use ampersand (&) selector to overwrite the few differences that apply to the `closed` element.


Something like this should work:

```suggestion
.insight {
  display: block;
  width: auto;
  height: auto;
  margin: 10px 0;
  padding: 10px;
  border-radius: 3px;
  overflow: hidden;
  border: 1px solid var(--sys-color-outline);
  background-color: var(--sys-color-base);
  &.closed { 
    background-color: var(--sys-color-neutral-container);
    border: none;
  }
}
```

And then on the `.closed` component it would be
`<div class="insight closed">`

Adriana Ixba

Done

Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement satisfiedCode-Review
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I758c279b0de7bd9ae2553a7d03aeed1d1af1221f
Gerrit-Change-Number: 5661588
Gerrit-PatchSet: 3
Gerrit-Owner: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Alina Varkki <alina...@chromium.org>
Gerrit-CC: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-CC: Nancy Li <nan...@chromium.org>
Gerrit-Comment-Date: Thu, 27 Jun 2024 18:29:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Alina Varkki <alina...@chromium.org>
satisfied_requirement
open
diffy

Devtools-frontend LUCI CQ (Gerrit)

unread,
Jun 27, 2024, 2:58:40 PM (3 days ago) Jun 27
to Adriana Ixba, Alina Varkki, Nancy Li, devtools-rev...@chromium.org

Devtools-frontend LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: front_end/panels/timeline/components/SidebarInsight.ts
Insertions: 5, Deletions: 5.

@@ -9,7 +9,7 @@
 
 export interface InsightDetails {
   title: string;
-  toggled: boolean;
+  expanded: boolean;
 }
 
 export class SidebarInsight extends HTMLElement {
@@ -17,11 +17,11 @@
   readonly #shadow = this.attachShadow({mode: 'open'});
   readonly #boundRender = this.#render.bind(this);
   #insightTitle: string = '';
-  #toggled: boolean = false;
+  #expanded: boolean = false;
 
   set data(data: InsightDetails) {
     this.#insightTitle = data.title;
-    this.#toggled = data.toggled;
+    this.#expanded = data.expanded;
   }
 
   connectedCallback(): void {
@@ -31,9 +31,9 @@
 
   #render(): void {
     let output: LitHtml.TemplateResult;
-    if (!this.#toggled) {
+    if (!this.#expanded) {
       output = LitHtml.html`
-        <div class="insight-closed">
+        <div class="insight closed">
             <h3 class="insight-title">${this.#insightTitle}</h3>
         </div>`;
     } else {
```
```
The name of the file: front_end/panels/timeline/components/Sidebar.ts
Insertions: 6, Deletions: 6.

@@ -88,7 +88,7 @@
   #activeTab: SidebarTabsName = SidebarTabsName.INSIGHTS;
   selectedCategory: InsightsCategories = InsightsCategories.ALL;
   #expanded: boolean = false;
-  #lcpPhasesToggled: boolean = false;
+  #lcpPhasesExpanded: boolean = false;
 
   #traceParsedData?: TraceEngine.Handlers.Types.TraceParseData|null;
   #inpMetric: {
@@ -167,7 +167,7 @@
     this.#insights = insights;
     this.#phaseData = this.getLCPInsightData();
     // Reset toggled insights.
-    this.#lcpPhasesToggled = false;
+    this.#lcpPhasesExpanded = false;
     void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#renderBound);
   }
 
@@ -288,7 +288,7 @@
   }
 
   #toggleLCPPhaseClick(): void {
-    this.#lcpPhasesToggled = !this.#lcpPhasesToggled;
+    this.#lcpPhasesExpanded = !this.#lcpPhasesExpanded;
     this.dispatchEvent(new ToggleSidebarInsights());
     void ComponentHelpers.ScheduledRender.scheduleRender(this, this.#renderBound);
   }
@@ -323,12 +323,12 @@
     const showLCPPhases = this.#phaseData ? this.#phaseData.length > 0 : false;
 
     // clang-format off
-    if (this.#lcpPhasesToggled) {
+    if (this.#lcpPhasesExpanded) {
       return LitHtml.html`${
         showLCPPhases ? LitHtml.html`
         <${SidebarInsight.SidebarInsight.litTagName} .data=${{
             title: lcpTitle,
-            toggled: this.#lcpPhasesToggled,
+            expanded: this.#lcpPhasesExpanded,
           } as SidebarInsight.InsightDetails}>
           <div slot="insight-description" class="insight-description">
             Each
@@ -350,7 +350,7 @@
       return LitHtml.html`
       <${SidebarInsight.SidebarInsight.litTagName} .data=${{
             title: lcpTitle,
-            toggled: this.#lcpPhasesToggled,
+            expanded: this.#lcpPhasesExpanded,
           } as SidebarInsight.InsightDetails}>
         </${SidebarInsight.SidebarInsight}>`;
 
```
```
The name of the file: front_end/panels/timeline/components/sidebarInsight.css
Insertions: 11, Deletions: 13.

@@ -9,22 +9,20 @@

   width: auto;
   height: auto;
   margin: 10px 0;
+  padding: 10px;
+  border-radius: 3px;
+  overflow: hidden;
   border: 1px solid var(--sys-color-outline);
-  padding: 10px;
-  border-radius: 3px;
-  overflow: hidden;
   background-color: var(--sys-color-base);
-}
 
-.insight-closed {
-  display: block;
-  width: auto;
-  height: auto;
-  margin: 10px 0;
-  padding: 10px;
-  border-radius: 3px;
-  overflow: hidden;
-  background-color: var(--sys-color-neutral-container);
+  &.closed {
+    background-color: var(--sys-color-neutral-container);
+    border: none;
+  }
+
+  &.closed:hover {
+    background-color: var(--sys-color-state-disabled-container);
+  }
 }
 
 .insight-title {
```
```
The name of the file: front_end/panels/timeline/components/SidebarInsight.test.ts
Insertions: 3, Deletions: 3.

@@ -14,7 +14,7 @@
 
   it('renders insight title', async () => {
     const component = new SidebarInsight();
-    component.data = {title: 'LCP by Phase', toggled: true};
+    component.data = {title: 'LCP by Phase', expanded: true};
     renderElementIntoDOM(component);
 
     await coordinator.done();
@@ -27,7 +27,7 @@
 
   it('renders only insight title when not toggled', async () => {
     const component = new SidebarInsight();
-    component.data = {title: 'LCP by Phase', toggled: false};
+    component.data = {title: 'LCP by Phase', expanded: false};
     renderElementIntoDOM(component);
 
     await coordinator.done();
@@ -44,7 +44,7 @@
 
   it('renders title, description and content when toggled', async () => {
     const component = new SidebarInsight();
-    component.data = {title: 'LCP by Phase', toggled: true};
+    component.data = {title: 'LCP by Phase', expanded: true};
     renderElementIntoDOM(component);
 
     await coordinator.done();
```

Change information

Commit message:
[RPP] Make sidebar insight clickable to toggle.

When clicked, the insight will expand to show its contents.
Bug: 349700397
Change-Id: I758c279b0de7bd9ae2553a7d03aeed1d1af1221f
Reviewed-by: Alina Varkki <alina...@chromium.org>
Commit-Queue: Adriana Ixba <ai...@google.com>
Files:
  • M front_end/panels/timeline/components/Sidebar.ts
  • M front_end/panels/timeline/components/SidebarInsight.test.ts
  • M front_end/panels/timeline/components/SidebarInsight.ts
  • M front_end/panels/timeline/components/sidebar.css
  • M front_end/panels/timeline/components/sidebarInsight.css
Change size: M
Delta: 5 files changed, 99 insertions(+), 10 deletions(-)
Branch: refs/heads/main
Submit Requirements:
  • requirement satisfiedCode-Review: +1 by Alina Varkki
Open in Gerrit
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: merged
Gerrit-Project: devtools/devtools-frontend
Gerrit-Branch: main
Gerrit-Change-Id: I758c279b0de7bd9ae2553a7d03aeed1d1af1221f
Gerrit-Change-Number: 5661588
Gerrit-PatchSet: 4
Gerrit-Owner: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Adriana Ixba <ai...@google.com>
Gerrit-Reviewer: Alina Varkki <alina...@chromium.org>
Gerrit-Reviewer: Devtools-frontend LUCI CQ <devtools-fro...@luci-project-accounts.iam.gserviceaccount.com>
Gerrit-CC: Nancy Li <nan...@chromium.org>
open
diffy
satisfied_requirement
Reply all
Reply to author
Forward
0 new messages