MDC Migration: Globally restyle `mat-icon-button` #6608

rileyajones · 2023-09-28T23:27:51Z

Motivation for features / changes

One of the biggest changes we have noticed during the mdc migration appears in the mat-icon-button. I am opting to attempt to fix them all at once by modifying the global files present in the theme file, then tweaking specific buttons.

Screenshots of UI changes (or N/A)

Before

Light Mode

Dark Mode

Extent Button

Table Header Buttons

After

Light Mode

Dark Mode

Extent Button

Table Header Buttons

After with Restyling

Light Mode

Dark Mode

Extent Button

Table Header Buttons

Googlers see

Light Mode
Dark Mode

bmd3k · 2023-09-29T15:51:34Z

tensorboard/webapp/theme/_tb_theme.template.scss

+
+      mat-icon.mat-icon,
+      svg {
+        width: 1em;


If I understand correctly, the affect of setting width and height to "1em" is that we can now control svg size by using "font-size" property?

That's interesting but I also think it's counter intuitive and probably the wrong path to go down. Consider:

Angular defines its icon and button sizes in terms of pixels.

We load our icons in terms of pixels (they are all loaded as 24px).

The large majority of our icons are displayed as 24px x 24px and that has been perfectly fine.

We really should just put:
height: 24px;
width: 24px;
To be consistent with the defaults of the current UI.

If there is a component that wants to specify a smaller icon (say, for example, the icons in the runs table) then they can just also specify their overrides in terms of pixels.

How do you feel about meeting in the middle with a css variable?

Yes, a css variable seems like a reasonable idea.

I don't have a lot of experience with them - is there a way to document the variable in a way that is discoverable and understandable by those who actually need to use it?

tensorboard/webapp/theme/_tb_theme.template.scss

bmd3k · 2023-09-29T16:56:56Z

tensorboard/webapp/theme/_tb_theme.template.scss

+      }
+
+      mat-icon.mat-icon {
+        min-width: 1em;


I think what you're really looking for is:

flex-shrink: 0;

You're right, that is cleaner

bmd3k · 2023-09-29T17:57:28Z

tensorboard/webapp/widgets/data_table/header_cell_component.scss

@@ -60,6 +50,18 @@ $_icon_size: 12px;
    line-height: 1;
  }

+  button.sorting-icon-container,


button[mat-icon-button].mat-mdc-icon-button { &.sorting-icon-container, &.context-menu-container { border-radius: 5px; font-size: 0; height: $_icon_size + 4px; padding: 0; width: $_icon_size + 4px; mat-icon, ::ng-deep svg { height: $_icon_size; line-height: 1; width: $_icon_size; } }

bmd3k · 2023-09-29T18:00:15Z

tensorboard/webapp/theme/_tb_theme.template.scss

@@ -291,6 +291,33 @@ $tb-dark-theme: map_merge(
    }
  }

+  body,


To summarize various comments below:

body, body.dark-mode { a[mat-icon-button].mat-mdc-icon-button, button[mat-icon-button].mat-mdc-icon-button { height: 40px; width: 40px; display: inline-flex; justify-content: center; align-items: center; .mat-mdc-button-touch-target { height: 100%; width: 100%; } mat-icon.mat-icon { flex-shrink: 0; } mat-icon.mat-icon, svg { height: 24px; width: 24px; } } }

tensorboard/webapp/widgets/line_chart_v2/sub_view/line_chart_axis_view.scss

bmd3k · 2023-09-29T18:05:25Z

tensorboard/webapp/metrics/views/_common.scss

@@ -61,6 +61,10 @@ $metrics-min-card-height: 320px;
 @mixin metrics-card-controls {
  @include tb-theme-foreground-prop(color, secondary-text);
  white-space: nowrap;
+
+  ::ng-deep button[mat-icon-button] {
+    font-size: 16px;


This is unnecessary with my other suggestions. I think the 24px x 24px default is perfectly fine here.

bmd3k · 2023-09-29T18:08:03Z

tensorboard/webapp/metrics/views/_common.scss

+
+  ::ng-deep button[mat-icon-button] {
+    font-size: 16px;
+  }


Somewhere in the metrics code we (unsuccessfully) try to override the height of the pin icon to 18px (I see it in the css browser but it doesn't get applied). Can you remove that as part of this change?

I think it looks perfectly fine at 24px x 24px. The reason it might look odd to someone is because the two icons directly surrounding it are a bit squat - but that's not really the pin icon's problem.

tensorboard/webapp/metrics/views/_common.scss

bmd3k · 2023-10-03T18:05:32Z

tensorboard/webapp/metrics/views/_common.scss

+
+  // ::ng-deep button[mat-icon-button] {
+  //   font-size: 16px;
+  // }


Somewhere in the metrics code we (unsuccessfully) try to override the height of the pin icon to 18px (I see it in the css browser but it doesn't get applied). Can you remove that as part of this change?

Can you address this comment I left previously?

Good catch on the pin icon size. I removed the failed overrides.

tensorboard/webapp/widgets/line_chart_v2/sub_view/line_chart_axis_view.scss

## Motivation for features / changes During the mdc-migration I restyled all the buttons via #6608 and only recently noticed that the `mat-calendar` component (which is only used internally) looked pretty off... To resolve this I am allowing button widths and heights to be set separately then overriding them in the internal date picker component (Googlers see cl/601592566). ## Screenshots of UI changes (or N/A) Before - https://screenshot.googleplex.com/AKuAQqzf9g6K2GF After - https://screenshot.googleplex.com/7s6vm8SsajKg9v7

globally restyle mat-icon-button

5e7b135

rileyajones requested a review from JamesHollyer September 28, 2023 23:27

rileyajones marked this pull request as ready for review September 28, 2023 23:29

rileyajones requested a review from bmd3k September 28, 2023 23:30

JamesHollyer approved these changes Sep 29, 2023

View reviewed changes

bmd3k reviewed Sep 29, 2023

View reviewed changes

rileyajones added 3 commits September 29, 2023 20:45

change icon size, and use css variable over em

e0bace3

remove redundant properties

12c46a1

fix extent button

6543aca

rileyajones requested a review from bmd3k October 3, 2023 17:58

bmd3k approved these changes Oct 3, 2023

View reviewed changes

address pr comments

d839bba

rileyajones merged commit 21bef1b into tensorflow:MDCMigration Oct 3, 2023

rileyajones mentioned this pull request Dec 21, 2023

Bug Fix: Allow icons to be sized by both width and height #6709

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDC Migration: Globally restyle `mat-icon-button` #6608

MDC Migration: Globally restyle `mat-icon-button` #6608

rileyajones commented Sep 28, 2023 •

edited

Loading

bmd3k Sep 29, 2023

rileyajones Sep 29, 2023

bmd3k Oct 2, 2023

bmd3k Sep 29, 2023

rileyajones Sep 29, 2023

bmd3k Sep 29, 2023

bmd3k Sep 29, 2023

bmd3k Sep 29, 2023

bmd3k Sep 29, 2023

bmd3k Oct 3, 2023

rileyajones Oct 3, 2023

@@ @@ -291,6 +291,33 @@ $tb-dark-theme: map_merge( @@
                   }
                 }
+                body,

MDC Migration: Globally restyle mat-icon-button #6608

MDC Migration: Globally restyle mat-icon-button #6608

Conversation

rileyajones commented Sep 28, 2023 • edited Loading

Motivation for features / changes

Screenshots of UI changes (or N/A)

Before

Light Mode

Dark Mode

Extent Button

Table Header Buttons

After

Light Mode

Dark Mode

Extent Button

Table Header Buttons

After with Restyling

Light Mode

Dark Mode

Extent Button

Table Header Buttons

Googlers see

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

MDC Migration: Globally restyle `mat-icon-button` #6608

MDC Migration: Globally restyle `mat-icon-button` #6608

rileyajones commented Sep 28, 2023 •

edited

Loading