fix(Button): Remove `cloneElement()`

[r100-stack]

Show more of comment (Only pertinent portion)

> ... (shortened) > > As for React, I think our users are already having issues with `React.cloneElement`, so we might want to change the approach to _always_ have a wrapping span. The DOM structure could look something like this: > ```html > > > > > Text > > ``` > > And then react could would just wrap the icon instead of cloning (similar change in IconButton): > > ```jsx > const Button = ({ startIcon, children }) => ( > > {startIcon} > {children} > > ); > ``` > > ... (shortened)

_Originally posted by @mayank99 in https://github.com/iTwin/iTwinUI/pull/768#discussion_r991647670_

---

• Removes React.cloneElement() from Button and IconButton. Instead uses a <span> wrapper

• Prerequisite: CSS PR #788

  • That PR styles the inner child of .iui-button-icon, instead of the class itself (Needed since now adding <span> wrapper)
    

• Test image changes are because of:

  • CSS PR #788: Set line-height: 1.2 and removed margin-block
  • CSS PR #796: Adds right margin to the arrow dropdown button for Select/ComboBox

Checklist

• ~[ ]~ Add meaningful unit tests for your component (verify that all lines are covered)
• [x] Verify that all existing tests pass
• ~[ ]~ Add component features demo in Storybook (different stories)
• [x] Approve test images for new stories
• ~[ ]~ Add screenshots of the key elements of the component

[r100-stack]

<SvgCloseSmall/> in Alert should also be wrapped in span.

I think there are other uses of <Svg...>s used in this repo that still use cloneElement and not using a wrapper <span>. E.g. TreeNode.tsx L270-275.

But I don't think we are removing cloneElement from there in this PR. We are only removing cloneElement by adding a wrapper span in Button and IconButton.

So should I just leave Alert.tsx as it is? Or maybe should I just add a span over just this one place of <SvgCloseSmall> in Alert but keep all other cloneElement and direct Svg... uses the same?

[mayank99]

@rohan-kadkol Alert needs the wrapping span for correct visuals (more noticeable in dark theme). You might want to go through all usages of the iui-button-icon mixin in CSS and update all of those in react.

[r100-stack]

@gretanausedaite @mayank99 I wrapped <SvgCloseSmall/> in Alert with a wrapper span and approved the changed Alert test images

[r100-stack]

@mayank99 @gretanausedaite @iui-button-icon mixin was @included in css in only three places: Alert, the close icon of Select Tags, and .iui-button-icon itself.

I tested Alert and confirmed it works alright in the dark theme. And Select tags are not there in react. I also went through all the stories of components in the css repo that had .iui-button-icon. All stories of those components work alright in the dark theme too.

[mayank99]

@rohan-kadkol Awesome! Just make sure you update your branch and run tests locally before merging.

[r100-stack]

Just make sure you update your branch and run tests locally before merging.

Thanks for reminding. All unit tests pass. Out of the failing image tests, I approved all except two as I felt they were flaky tests. Because when I tested them manually in the story, they produced the baseline image and not the failing test image.

The two images I did not approve are: Header.test.ts-Full and the usual Select.test.ts-Truncate Middle Text (Closed)