-
Notifications
You must be signed in to change notification settings - Fork 129
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
Text field Rewrite #701
base: main
Are you sure you want to change the base?
Text field Rewrite #701
Conversation
Appreciate the contribution, but I have a few requests:
No rush on any of this, since I likely won't merge any other PRs until after I get #694 finished |
Per this stack overflow thread, using a With that in mind, it also isn't idiomatic to pass the controller to the widget, as the lifecycle of the controller is managed by the I think for the current usage it's slightly better to take a number, as determining the number of decimals to display is something that the component is good at abstracting. We could add a The main reason I used an enum was to avoid having to invent an arbitrarily-small-but-not-zero number to replace the >0 case. I have seen other software do that, however, so I don't have a problem making that change. |
This PR fixes the following bugs/strange behaviors associated with the
NumberTextField
component and theRenamableTitle
component:Other minor tweaks/changes this PR makes include:
NumberTextField
is now a controlled component, taking and rendering a value directly.NumberTextField
no longer takes a controller.NumberTextField
now takes alowerBound
as an argument to clean up its usage.This is my first time working with Flutter/Dart, so definitely don't hesitate to point out any issues/flaws you see! I'm especially curious to see if there's a good way to combine the common code between
NumberTextField
andRenamableTitle
- I took a crack at it with an abstract class extendingState
, but it was pretty ugly. Not sure if there's a better way extendingTextField
or something else.