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

dynamic padding has default value 10.0. Change it to 0.0 #127

Closed
nikpelgr opened this issue Sep 14, 2024 · 6 comments
Closed

dynamic padding has default value 10.0. Change it to 0.0 #127

nikpelgr opened this issue Sep 14, 2024 · 6 comments

Comments

@nikpelgr
Copy link

nikpelgr commented Sep 14, 2024

Hi,
in this commit(6c05345) you changed padding to be dynamic padding with default value 10.0.
The property 'padding' and 'fieldDecoration' cannot co-exist, meaning when someone wants to decorate the widget, doesn't have the option to add padding. I couln't find a way to use padding in Decoration class or BoxDecoration until now.

I would suggest to use default value 0.0 and let someone with a Padding widget to include the search_choices widget and pad it.
As workaround, I manually changed the default value in #1390 but in the next update, I will loose it.

@lcuis
Copy link
Owner

lcuis commented Sep 15, 2024

Hi @nikpelgr ,

Thanks a lot for your message. I admit this is a very old topic for me so I may be wrong in my reading, understanding and answering. Sorry for that.

The commit 6c05345 you are referring to doesn't seem to indicate that the default value for padding changed from 0 to 10 (at least not with this commit). Instead, it seems that padding type was changed from double to dynamic so that it can support double or EdgeInsets.

When I read the following code from search_choices.dart, I believe your mention of an incompatibility between padding and fieldDecoration is true:

    assert(fieldDecoration == null || padding == null,
        "use either padding or fieldDecoration");

I believe you would like to use fieldDecoration and not padding. If so, I would expect that setting padding to null and fieldDecoration to the desired value to be an effective way to achieve that.
Maybe that's not the case. If so, can you please let me know. I will try to look into this in details.

Cheers!

@nikpelgr
Copy link
Author

Hi,
What you are writing is totally different from what I wanted to say. My mistake. Let me try again.
In the commit that I mentioned, it is exactly as you described.
Now, when I am trying to use padding and also decorate the widget, I get an error which is described in the above portion of code. And so, keeping the padding parameter away, I have a widget with default padding as 10.0 on all sides. The rest of the widgets in the screen, have LTRB(5, 0, 5, 0) and so you can image how different your widget looks.
In the fieldDecoration field, I used BoxDecoration in order to define the Borders and in final, it was difficult to find another class for Decoration that has padding.

So, I dived into your code and on line #1390 I changed the default 10.0 -> 0.0 and this allowed me to have search_choices as a child to a Padding widget with LTRB (5, 0, 5, 0). Job done.

That's why I suggest this value to be 0.0 as default.

@lcuis
Copy link
Owner

lcuis commented Sep 15, 2024

Oh, I think I get it, thanks for your further explanations and sorry for not getting it the first time.

There is indeed a padding default to 10 in case padding is null so my previous answer doesn't address your issue since, in addition, you want to set fieldDecoration while padding is anyway used in another portion of the code.

    EdgeInsets treatedPadding = widget.padding is EdgeInsets
        ? widget.padding
        : EdgeInsets.all(widget.padding is int
            ? widget.padding.toDouble()
            : widget.padding ?? 10.0);

In that case, maybe the assert mentioned in my previous comment is not the best way to deal with this padding vs fieldDecoration question?

If I were to remove the assert, would that solve your issue you think?

The thing with removing the padding default (or setting it to 0) is that this would change the behavior for other users of the plugin and the automated non regression testing (I could easily deal with the latter honestly).

@nikpelgr
Copy link
Author

nikpelgr commented Sep 15, 2024

Yes,
restored the default to 10.0 and commented out the assert. Now, I have an active field padding = 0 and it works as it should be.

@lcuis
Copy link
Owner

lcuis commented Sep 16, 2024

Great!

I'm about to publish release 2.2.9 which should solve your issue then.
I'm just running the automated non-regression testing before that.

@lcuis
Copy link
Owner

lcuis commented Sep 16, 2024

Release 2.29 is now published.

Feel free to close this issue if solved.

Cheers!

@lcuis lcuis closed this as completed Sep 16, 2024
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

No branches or pull requests

2 participants