Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support icon as textfield prefix/suffix #76

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vsandvold
Copy link
Contributor

This PR adds a specialized kind of Affix component, that renders an icon as prefix/suffix in a Textfield.

I'm happy to discuss the interface for this new component. It would also be possible to split out the existing Affix into separate LabelAffix and ButtonAffix component, possibly making the code a bit clearer. Please let me know if you are open to discuss this idea, or if you prefer to incorporate icons in a different way (if at all).

@benja
Copy link
Contributor

benja commented Jan 17, 2022

Let's discuss this in tomorrow's meeting? I think it'd fit perfectly in the combobox discussion.

@vsandvold
Copy link
Contributor Author

vsandvold commented Mar 16, 2022

Please feel free to close this PR is you have decided not to accept the proposed changes. I think you should consider it though, so let me try to explain 😄

When I was working on the Affix component, I noticed if-else blocks that toggle different kinds of behavior inside the component. Along the lines of "if this prop, then render this kind of content, and if that prop, render some other kind of content". Components (and classes) with conditional behavior is a kind of code smell, and to me this looks like an opportunity to introduce some polymorphism. With a few, more specialized components, the user of the component can decide what kind of (specialized) Affix is needed, and the components themselves can focus on separate concerns (ie. render a label, or a button, or an icon). If you want to re-use some common Affix behavior across these specialized components, you can do this with composition. It also becomes easier to extend the Affix component with more specialized behavior when it is split like this.

If you want to make this change, you could perhaps introduce the new components while deprecating the old Affix, and then remove it in a future major version release?

But then again, this is just a suggestion. I have currently solved then problem by creating a custom IconAffix component in our own codebase, and using that together with the Fabric Combobox and TextField components. That works fine as long as the Affix interface remain the same in Fabric, but it would be better for us to have this implemented directly in Fabric, of course.

I hope that was useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants