iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
84 stars 23 forks source link

chore: Update to using itwinui-css 1.0.0-dev.9 #881

Closed r100-stack closed 2 years ago

r100-stack commented 2 years ago

Checklist

mayank99 commented 2 years ago

I've also released 1.0.0-dev.9 if you want to update that in this PR. It includes changes from https://github.com/iTwin/iTwinUI/pull/772.

r100-stack commented 2 years ago

I've also released 1.0.0-dev.9 if you want to update that in this PR. It includes changes from iTwin/iTwinUI#772.

Thanks, updated title to mention updating to say "1.0.0-dev.9" and also approved the test images from 1.0.0-dev.9's CSS PR #772

r100-stack commented 2 years ago

Also make sure that all tests are passing before you merge this.

Thanks, yes, all units tests are passing. For visual tests, I approved all test changes except the two Select Truncate Middle Text story images.

I think the Select Truncate text story is a bit flaky and the image might need to be reverted.

Reverted the two Select Truncate Middle Text story images. I have been seeing this test fail a couple of times before too. I will keep a look for it in the future too when approving, since, like you said, this looks like a flaky test.

mayank99 commented 2 years ago

Table Cell and Row status image should not have changed. We need to fix colours here or in css. image

@gretanausedaite Are you sure? I see changes in https://github.com/iTwin/iTwinUI/pull/772/files.

gretanausedaite commented 2 years ago

@gretanausedaite Are you sure? I see changes in iTwin/iTwinUI#772 (files).

Yes, icons should be grey only when row is disabled (or loading). I think new css is adding fill to svg and it's overriding svg styles added in React. In css we did not see this issue because there svg has fill colour written in path.

r100-stack commented 2 years ago

@gretanausedaite Are you sure? I see changes in iTwin/iTwinUI#772 (files).

Yes, icons should be grey only when row is disabled (or loading). I think new css is adding fill to svg and it's overriding svg styles added in React. In css we did not see this issue because there svg has fill colour written in path.

Thanks for pointing that out. In React, we were adding the fill to the SVG as an "attribute style". This gave it a low specificity and thus CSS repo fill was being used.

I now changed it to add fill to the SVG as an "inline style" (Commit). Now if the fill inline style is provided, it uses that, else it uses the default CSS repo fill (L452).

Table Cell and Row status image should not have changed

The new test image reverted that change

mayank99 commented 2 years ago

@rohan-kadkol This still needs a CSS fix, as we can't expect users to always provide the fill as an inline style.

r100-stack commented 2 years ago

@rohan-kadkol This still needs a CSS fix, as we can't expect users to always provide the fill as an inline style.

Sorry, I saw two approvals after my push, so I merged. Maybe next time I'll wait a little longer just to confirm everything is alright.

I will work on that CSS change. When confirming that CSS change in react, I will undo the inline style changes from this commit.