`.iui-field` handles icon fill #2088

FlyersPh9 · 2024-06-04T18:12:43Z

Changes

Add 3 .iui-field variables:
- --_iui-field-color-icon
- --_iui-field-color-icon-hover
- --_iui-field-color-icon-disabled
Colors inherited in button and select.

Testing

Ran visual tests.

Docs

n/a

packages/itwinui-css/src/select/select.scss

Ben-Pusey-Bentley

I am a bit confused about the purpose of this PR. The conversation that this PR is in response to seems to be about adding the icon fill logic into the field file, but this PR only adds a few variables to that file, and leaves all of the logic setting icon fill in the button and select files. Is this what we want?

mayank99

Adding to @Ben-Pusey-Bentley's comment, this PR looks half-baked. iui-field needs to handle the actual icon fill, most importantly the fact that it changes between default, :hover, and disabled states.

FlyersPh9 · 2024-06-04T20:28:00Z

iui-field needs to handle the actual icon fill

Would it be acceptable to add an .iui-field-icon class? Button uses .iui-button-icon and Select uses .iui-icon, I think we'll need a shared class to apply the fill correctly.

mayank99 · 2024-06-05T13:24:05Z

Would it be acceptable to add an .iui-field-icon class?

@FlyersPh9 I don't like the idea of a iui-field-icon class. We already have too many icon classes that need to be consolidated before we add another one.

For this PR, it might be simpler to set the fill in iui-field and inherit it wherever it needs to be used. This way, we get to keep the markup unchanged.

.iui-field {
  // …
  fill: var(--_iui-field-state--default, var(--_iui-field-color-icon))
    var(--_iui-field-state--hover, var(--_iui-field-color-icon-hover))
    var(--_iui-field-state--disabled, var(--_iui-field-color-icon-disabled));
}

.iui-button-icon svg {
  fill: inherit;
}

.changeset/little-meals-pay.md

FlyersPh9 · 2024-06-05T20:54:50Z

packages/itwinui-css/src/tile/tile.scss

+  .iui-button[data-iui-variant='borderless'] {
+    border-radius: var(--iui-border-radius-round);

-    > .iui-button-icon {
-      filter: drop-shadow(0 2px 1px rgba(0, 0, 0, var(--iui-opacity-5)));
-    }
+    .iui-tile-thumbnail-picture ~ & {
+      @include mixins.iui-blur($opacity: 5);
+      --_iui-field-color-icon: var(--iui-color-white);
+      --_iui-field-color-icon-hover: var(--iui-color-white);
+
+      > .iui-button-icon {
+        filter: drop-shadow(0 2px 1px rgba(0, 0, 0, var(--iui-opacity-5)));
+      }

-    &:hover,
-    &[data-iui-active='true']:enabled,
-    &[data-iui-active='true']:enabled:hover,
-    &[data-iui-active='true']:active {
-      background-color: rgba(0, 0, 0, var(--iui-opacity-4));
+      &:hover,
+      &[data-iui-active='true']:enabled,
+      &[data-iui-active='true']:enabled:hover,
+      &[data-iui-active='true']:active {
+        background-color: rgba(0, 0, 0, var(--iui-opacity-4));
+      }


Tile was failing React tests. Looked into it and our CSS demo's HTML structure was not matching React's.

React:

<div class='iui-tile-thumbnail-quick-action'> <button class="iui-button iui-button-base iui-field">…</button> </div>

CSS:

<button class="iui-button iui-button-base iui-field iui-tile-thumbnail-quick-action">…</button>

Made the changes in CSS demos to match React & had to update the CSS to work properly. The hover state of the buttons overlaying the thumbnail aren't even working properly in prod right now: https://itwin.github.io/iTwinUI/react/?mode=preview&story=tile--all-props

If this is a visual bug fix, we would need css and react changesets for it.

Also, just wondering, since we corrected the css demo page to match what it used to be in react, could that change in the DOM be a breaking change for itwinui-css?

I added this to the changesets. Unsure if this warrants a major or not.

FlyersPh9 · 2024-06-05T20:57:27Z

For this PR, it might be simpler to set the fill in iui-field and inherit it wherever it needs to be used. This way, we get to keep the markup unchanged.

This is done. @mayank99 & @Ben-Pusey-Bentley, I think this could use another review now.

packages/itwinui-css/src/button/base.scss

packages/itwinui-css/src/tile/tile.scss

Ben-Pusey-Bentley

Looking at your CI output, it looks like the changes that you have made to the CSS tile have also introduced some minor visual changes. It seems to be something with the border of the icon buttons. May want to tweak the CSS tile some more to avoid these visual changes, or you may want to approve some new test images, if you feel that the visual change is fine to keep. Diff:

packages/itwinui-css/src/select/select.scss

packages/itwinui-css/src/tile/tile.scss

FlyersPh9 · 2024-06-06T16:42:01Z

packages/itwinui-css/src/tile/tile.scss

-    @include mixins.iui-blur($opacity: 5);
-    --_iui-button-icon-fill: var(--iui-color-white);
+  .iui-tile-thumbnail-button {
+    border-radius: 50%;


Suggested change

border-radius: 50%;

border-radius: var(--iui-border-radius-round);

Feel like this should be changed, but will likely effect visuals tests. If we like this change, I'll make it right at the end to keep image test changes until the last push.

mayank99

LGTM

apps/react-workshop/cypress-visual-screenshots/baseline/Tile.test.ts-All Props.png

.changeset/small-otters-itch.md

field class handles icon fill

dac6097

FlyersPh9 self-assigned this Jun 4, 2024

FlyersPh9 mentioned this pull request Jun 4, 2024

use cyclic state toggles in field CSS #2075

Merged

FlyersPh9 commented Jun 4, 2024

View reviewed changes

packages/itwinui-css/src/select/select.scss Outdated Show resolved Hide resolved

FlyersPh9 marked this pull request as ready for review June 4, 2024 18:23

FlyersPh9 requested review from a team as code owners June 4, 2024 18:23

FlyersPh9 requested review from r100-stack and Ben-Pusey-Bentley and removed request for a team June 4, 2024 18:23

Ben-Pusey-Bentley reviewed Jun 4, 2024

View reviewed changes

mayank99 reviewed Jun 4, 2024

View reviewed changes

r100-stack reviewed Jun 5, 2024

View reviewed changes

.changeset/little-meals-pay.md Outdated Show resolved Hide resolved

FlyersPh9 added 2 commits June 5, 2024 11:09

Field icon fill is inherited

801825c

Fix tile issue that was failing React tests.

e54f9c6

FlyersPh9 commented Jun 5, 2024

View reviewed changes

FlyersPh9 requested review from mayank99, Ben-Pusey-Bentley and r100-stack June 5, 2024 20:57

mayank99 reviewed Jun 5, 2024

View reviewed changes

packages/itwinui-css/src/button/base.scss Outdated Show resolved Hide resolved

packages/itwinui-css/src/tile/tile.scss Outdated Show resolved Hide resolved

Ben-Pusey-Bentley reviewed Jun 6, 2024

View reviewed changes

r100-stack reviewed Jun 6, 2024

View reviewed changes

packages/itwinui-css/src/select/select.scss Show resolved Hide resolved

packages/itwinui-css/src/tile/tile.scss Outdated Show resolved Hide resolved

FlyersPh9 added 2 commits June 6, 2024 11:59

Changes from comments.

7bc2cc9

Merge branch 'main' into jon/field-handles-icon-fill

a36dc68

FlyersPh9 commented Jun 6, 2024

View reviewed changes

Merge branch 'main' into jon/field-handles-icon-fill

76247f0

Removing tile changes.

74900a4

FlyersPh9 mentioned this pull request Jun 12, 2024

Fix tile thumbnail buttons after .iui-field #2101

Merged

FlyersPh9 added 2 commits June 12, 2024 14:29

Missed file.

5c2dcb4

Temporary fix to tile test image issue in React.

73cb4ed

mayank99 approved these changes Jun 12, 2024

View reviewed changes

FlyersPh9 requested review from Ben-Pusey-Bentley and r100-stack June 12, 2024 19:10

Ben-Pusey-Bentley reviewed Jun 12, 2024

View reviewed changes

apps/react-workshop/cypress-visual-screenshots/baseline/Tile.test.ts-All Props.png Outdated Show resolved Hide resolved

Trying to revert test image.

25ff29a

Ben-Pusey-Bentley approved these changes Jun 12, 2024

View reviewed changes

r100-stack approved these changes Jun 12, 2024

View reviewed changes

.changeset/small-otters-itch.md Outdated Show resolved Hide resolved

FlyersPh9 added 2 commits June 12, 2024 16:14

Removed unnecessary bullet point in changeset

7dba947

Merge branch 'main' into jon/field-handles-icon-fill

9f769ac

FlyersPh9 enabled auto-merge (squash) June 12, 2024 20:30

Merge branch 'main' into jon/field-handles-icon-fill

9578686

FlyersPh9 mentioned this pull request Jun 12, 2024

add "open" popover styling to buttons #2076

Merged

FlyersPh9 merged commit e131ed7 into main Jun 12, 2024
16 checks passed

FlyersPh9 deleted the jon/field-handles-icon-fill branch June 12, 2024 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`.iui-field` handles icon fill #2088

`.iui-field` handles icon fill #2088

FlyersPh9 commented Jun 4, 2024 •

edited

Loading

Ben-Pusey-Bentley left a comment

mayank99 left a comment •

edited

Loading

FlyersPh9 commented Jun 4, 2024

mayank99 commented Jun 5, 2024

FlyersPh9 Jun 5, 2024

r100-stack Jun 6, 2024

FlyersPh9 Jun 6, 2024

FlyersPh9 commented Jun 5, 2024

Ben-Pusey-Bentley left a comment

FlyersPh9 Jun 6, 2024

mayank99 left a comment

	border-radius: 50%;
	border-radius: var(--iui-border-radius-round);

.iui-field handles icon fill #2088

.iui-field handles icon fill #2088

Conversation

FlyersPh9 commented Jun 4, 2024 • edited Loading

Changes

Testing

Docs

Ben-Pusey-Bentley left a comment

Choose a reason for hiding this comment

mayank99 left a comment • edited Loading

Choose a reason for hiding this comment

FlyersPh9 commented Jun 4, 2024

mayank99 commented Jun 5, 2024

FlyersPh9 Jun 5, 2024

Choose a reason for hiding this comment

r100-stack Jun 6, 2024

Choose a reason for hiding this comment

FlyersPh9 Jun 6, 2024

Choose a reason for hiding this comment

FlyersPh9 commented Jun 5, 2024

Ben-Pusey-Bentley left a comment

Choose a reason for hiding this comment

FlyersPh9 Jun 6, 2024

Choose a reason for hiding this comment

mayank99 left a comment

Choose a reason for hiding this comment

`.iui-field` handles icon fill #2088

`.iui-field` handles icon fill #2088

FlyersPh9 commented Jun 4, 2024 •

edited

Loading

mayank99 left a comment •

edited

Loading