Implemented Icon.WithSize(int? size) enabling Icons to be resized...

[EricOnGit]

… to any dimensions.

Pull Request

📖 Description

This is the quick and easy implementation that was discussed in issue #1124.
It offers a new WithSize() function that enables the dev to override the width of the Icon to any size.

This is compatible with previous version since the OverriddenWidth value is initialized to null and then tested against at render time so it's only used if the value was changed.

🎫 Issues #1124

👩‍💻 Reviewer Notes

Comments were added directly in the Icon.cs file to better explain the change.

📑 Test Plan

There are currently no test on Icons. But I did test the component in my current project using both a FluentIcon and a FluentButton without any adverse effect.

✅ Checklist

General

• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

⏭ Next Steps

none to my knowledge

[EricOnGit]

@microsoft-github-policy-service agree

[dvoituron]

Thanks. Very cool improvement :-)

We did not have time to put in all the unit tests at the start of the project, but we try to add them with each component modification. So I'm going to add a few unit tests to Dev. This will allow you to add a new one for this method :-)

[dvoituron]

I think it would be possible to have a string argument: WithSize(string ? size) as in the ToMarkup method. This could be interesting for using % or other unit types. What do you think?

[EricOnGit]

Certainly, it would minimally require changes in the FluentIcon component and that exact ToMarkup method where "px" is hardcoded. It might also require changes in the way Width and Size are delt with (mostly Width which is a protected in an unsealed class). And that's where I decided to play it safe due to my limited knowledge of the ripple effects.

That said, if you're ok with pushing this a little further, I can spend some more time on this.
I would also be a bit more comfortable if we had some tests coverage here. Looks like we'll have to synch our changes to move forward😊

[dvoituron]

I added a PR #1136 to include Unit Tests

[EricOnGit]

I think you meant #1140

[vnbaaij]

Why not have both WithSize(int? size) and WithSize(string? size)?

And why are these parameters nullable? What would be the outcome of sending in a null value? I think we may expect people to supply a valid input value.

[dvoituron]

We could have both, with WithSize(int? size) => WithSize(size is null ? null : $"{size}px");
The advantage of having a null value is to "reset" another specified size.

[EricOnGit]

@dvoituron @vnbaaij I've added a new commit this morning that includes the changes to the generator so you can have a look at this code too.

[vnbaaij]

@EricOnGit, please revert that last commit and create a new PR for it. I'm fairly certain we will mergi in the WithSize work but not certain yet of the rest. Having it in a seperate PR allows us to evealuate things easier.

[vnbaaij]

Hold off a bit on creating the new PR please

[vnbaaij]

Ok, let's add the sting variant and get this merged in.

[EricOnGit]

Ok, let's add the sting variant and get this merged in.

@vnbaaij there is a catch to bringing the string variant.

The Width property of the Icon class is exposes as "protected internal". I'll admit I had to look it up to be sure. It turns out those modifiers are acting as OR. Old reference here

Which means that if we implement WithSize(string?) we will either have a messy meaning for Width (only the int part or something like "1.3rem"?) or have to expose Width as string which would break any derived class 😣

I thought of it for a moment and I see a couple options (none are perfect tho)

1. Introduce a Unit property that would support things like: rem, em, %. It feels like a can of worms at the bottom of a rabbit hole to me but it's an option nonetheless.

2. Introduce something like "bool UseParentWidth" or "bool FullWidth" to only support the use case of 100%. I think this is the most likely reason not to stick with pixels.

It's probably inevitable that this will have to break at some point but is this PR the right one? I rarely advise for caution but you mentioned earlier that you rather not introduce a breaking change.