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

Fixed the bugs with the drop down #1459

Merged
merged 4 commits into from
Jul 18, 2024
Merged

Fixed the bugs with the drop down #1459

merged 4 commits into from
Jul 18, 2024

Conversation

hemish11
Copy link
Contributor

Closes #1342 and #1292

@hemish11 hemish11 self-assigned this Jun 27, 2024
@@ -351,6 +353,7 @@ class SelectOneState extends FormFieldWidgetState<SelectOne>

return DropdownButtonFormField2<dynamic>(
key: validatorKey,
isExpanded: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we really hardcode isExpanded: true like this, this will make it expand to take all the space of the parent along the mainaxis. That should not be hardcoded right?

@vusters what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kmahmood74 This one is primarily there to ensure that the child, ie the drop-down entities are properly in the size of the dropdown like it makes the items expanded and thus it resolves that issue in which the item was overflowing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know why it's there but what is the impact of this on the actual layout? I am not sure if we can hardcode isExpanded like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it wont affect the layout, I did tested that one

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hemish11 please test the isExpanded: true with long text. Create a regular dropdown and an auto-complete dropdown each with items with very long labels that overflow multiple lines. Try it with and without isExpanded: true and report the results.

@@ -285,6 +285,8 @@ class SelectOneState extends FormFieldWidgetState<SelectOne>
validatorKey.currentState!.validate();
}
});*/
textEditingController = TextEditingController(text: widget.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few issues here -

  1. textEditingController needs to be disposed off in dispose
  2. you are setting the initial value in initState but what about updating the value as the value changes? value could be set at anytime in EDL. Also what if user selects a value, shouldn't the textEditingController be updated with that?
    Please test this thoroughly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll add that to the dispose method, and also testing that value of

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think about it Hemish - you are setting the value in initState which is called only once. How will it work when the developer changes value in js after the widget has already been built. Have you tested that? Can you please how it works?

@@ -285,6 +285,8 @@ class SelectOneState extends FormFieldWidgetState<SelectOne>
validatorKey.currentState!.validate();
}
});*/
textEditingController = TextEditingController(text: widget.getValue());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also check the getValue code which in turn calls isValueInItems which has the following -

      if (_controller.itemTemplate != null) {
        // TODO: we have no way to look into the itemTemplate to see
        // if the value matches one of them. Return true for now
        return true;
      }

looks like it just skips over when it is itemTemplate. Not sure how it will impact your fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, Vu wrote this one, and yes we don't have a way by which we can get items from the item template, which is the same issue by which we cannot add autoCorrect with item template

@@ -246,6 +249,8 @@ class SelectOneController extends FormFieldController with HasTextPlaceholder {
SelectOneInputFieldAction? inputFieldAction;
List<SelectOneItem>? items;

TextEditingController textEditingController = TextEditingController();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be handled internally in the widget, there is no need to expose it in the controller

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to add it to the controller so that its available in the setters

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need it to be available in the setters. You can update the value in the onchange event inside the state.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when user selects a value from the dropdown? You are setting the value when it is set in setters but what about when it is selected by user interaction? Will the textEditingController update, I don't see the code?

@@ -73,7 +73,10 @@ abstract class SelectOne extends StatefulWidget
Map<String, Function> setters() {
var setters = _controller.textPlaceholderSetters;
setters.addAll({
'value': (value) => _controller.maybeValue = value,
'value': (value) {
_controller.textEditingController.value = TextEditingValue(text: value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be handled internally in the widget, there is no need to expose it in the controller

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hemish11 I am not sure why this is needed as you just override and create a new one inside initState everytime? That was my point

@@ -285,6 +289,9 @@ class SelectOneState extends FormFieldWidgetState<SelectOne>
validatorKey.currentState!.validate();
}
});*/
widget.controller.textEditingController =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just set value, why create a new textEditingController?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because to set the initial value of a text field, we must need to pass a TextEditingController so we cannot just update a value variable

@@ -246,6 +249,8 @@ class SelectOneController extends FormFieldController with HasTextPlaceholder {
SelectOneInputFieldAction? inputFieldAction;
List<SelectOneItem>? items;

TextEditingController textEditingController = TextEditingController();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when user selects a value from the dropdown? You are setting the value when it is set in setters but what about when it is selected by user interaction? Will the textEditingController update, I don't see the code?

@kmahmood74 kmahmood74 merged commit 8e8c6f3 into main Jul 18, 2024
2 checks passed
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.

Cannot set value on a dropdown with autocomplete
2 participants