filament icon indicating copy to clipboard operation
filament copied to clipboard

Published 20 hours ago • verified publisher icon filamentphp

feature: Move text input affixes inside border

Open fira03079508 opened this issue 2 years ago • 9 comments

This is my first Pull Request on any project and actually my first time using TailwindCSS.

What I have in mind is that prefixIcon and suffixIcon should ideally be inside the input and not outside. I've changed some classes to allow that putting a pl-10 only if there's a prefixIcon (so you don't write inside the icon)

Maybe we should approach this as an option and maybe allow this for prefix / suffix and maybe even prefixAction / suffixAction

As it's my first time using TailwindCSS. I don't know the impact of changing the outer div to relative instead of the classes that were there

image

So here's my first attempt

fira03079508 avatar Sep 02 '22 00:09 fira03079508

If we're going to move the prefix icon into the box, we should consider moving the prefix label as well. Have a look at how Tailwind UI handles this: https://tailwindui.com/components/application-ui/forms/form-layouts#component-db11f83176d113e39bf2559da9344b1c.

Personally I like the design of containing everything within the bordered box better than having the affixes outside.

zepfietje avatar Sep 02 '22 07:09 zepfietje

Yes, I agree.

danharrin avatar Sep 02 '22 08:09 danharrin

Does this mean I should bring the changes to a new commit or these are just things that could be improved upon whoever does it? As I said above I'm really not experienced with this.

fira03079508 avatar Sep 02 '22 19:09 fira03079508

Does this mean I should bring the changes to a new commit or these are just things that could be improved upon whoever does it? As I said above I'm really not experienced with this.

You can just work on this PR/branch to implement mentioned improvements. If you need help, we can take over.

zepfietje avatar Sep 03 '22 12:09 zepfietje

I've tried adding this to labels also (not just Icons) and I had some problems with my Tailwind config. I wasn't able to check if everything was working correctly... In theory from what I know so far it should work but if someone could check if it works nicely visually please do.

(Couldn't see the effects of "ltr" modifier and also no rounded-l-XX or rounded-r-XX)

This is pretty much the extent of what I can provide at the moment. Will try to improve for my next PR :D

fira03079508 avatar Sep 04 '22 20:09 fira03079508

Is it possible to move the rounded classes to the wrapping div? I think it should, but I'm not sure just judging from the code.

Not sure, It's my first time using Tailwind. I've tried my best and wouldn't be offended at all if anything here is changed but I don't really know what I could do. Also i'm having issues testing my changes at the moment

fira03079508 avatar Sep 04 '22 20:09 fira03079508

Is it possible to move the rounded classes to the wrapping div? I think it should, but I'm not sure just judging from the code.

Not sure, It's my first time using Tailwind. I've tried my best and wouldn't be offended at all if anything here is changed but I don't really know what I could do. Also i'm having issues testing my changes at the moment

Alright, we'll have a look later!

zepfietje avatar Sep 04 '22 20:09 zepfietje

@danharrin, do you think this can be added to v2?

This is quite a design change, so might hold off for v3?

zepfietje avatar Sep 16 '22 10:09 zepfietje

Yeah, let's hold off on merging until the Design team has reviewed this as part of wider changes in v3

danharrin avatar Sep 16 '22 10:09 danharrin

Closing this PR as v3 will require it to target a different branch.

We're also tracking this on our roadmap: https://github.com/orgs/filamentphp/projects/2/views/6?query=is%3Aopen+sort%3Aupdated-desc.

zepfietje avatar Oct 08 '22 10:10 zepfietje