Multiplicom / axiom

3 stars 0 forks source link

Add option to droplist control to add a blank state only if there is more than 1 option #79

Closed MichaelVyverman closed 3 years ago

MichaelVyverman commented 3 years ago

Adds a settings option to the Droplist control to add a blank state with the default name "-- Select --" to the Droplist only if there is more than 1 option available. There is a second option to change this default name of the blank option.

Closes #78

To reviewers: would it make sense to also add this option only in case the initial value of the droplist is not part of the selected states (e.g. the DropList has been created without an initial selected value or the initial selected value is not known?

goranvinterhalter commented 3 years ago

I understand what you are trying to do, but it seams a bit over-engineered. I looked into djangos forms.ChoiceField to see how they implemented this logic. There, the 2 most important parameters are:

Then the logic is:

As far as I can see the combination of choices and required is enough to model all use cases I can think of. if I where to make it easier to add choice ("", "-- Select --"), as you are trying to do, then I would rather subclass a ChoiceField then complicate it's constructor. Or maybe a better approach would be to have an enum of possible NoValue tuples. In your case that would be having some pre-generated states like State.Select which has id='' and name="-- Select --". And if the user really really wants to have a custom message just for that one select, then he can just define an empty State on the spot.

MichaelVyverman commented 3 years ago

Thanks for the feedback

  • If you want to have a "-- Select --" you would add it to choices as ("", "-- Select --") and specify initial="" if that should be initial value.

This is what I want to avoid: to always have to add this option myself. This is how it is currently implemented and requires a lot of code duplication and has the disadvantage to be error prone to typos e.g. missing spaces, lower case, ...

  • But that is just a visual thing.

The visual thing is the following: if the list of choices contains only 1 item, the user has no option obviously and the ("", "-- Select --") should not be present

The user still has to make a choice as validation would fail. Therefore to make this into an optional value you would also have to specify required=False, that way "" is a valid value.

Given validation is not part of the control itself, this cannot be added to the control.

As far as I can see the combination of choices and required is enough to model all use cases I can think of. if I where to make it easier to add choice ("", "-- Select --"), as you are trying to do, then I would rather subclass a ChoiceField then complicate it's constructor.

I could do that: could create DropListWithDefaultOption and DropListWithoutDefaultOption, but I see this becoming very complicated for every combination of parameter options to create a new subclass

Or maybe a better approach would be to have an enum of possible NoValue tuples. In your case that would be having some pre-generated states like State.Select which has id='' and name="-- Select --". And if the user really really wants to have a custom message just for that one select, then he can just define an empty State on the spot.

I don't get this last suggestion

MichaelVyverman commented 3 years ago

I probably miss a lot of context so ignore my comment if it doesn't make sense :-), but I would assume that having a blank option also allows a field to be optional. Based on that I would not add it only in case the initial value of the dropdown is not part of the selected states.

Almost: having a blank option does not allow the field to be optional, but the reverse is true in 90% of the cases. Having a blank option can also mean the field is mandatory but no sensible value can be used as initial value.

goranvinterhalter commented 3 years ago

Or maybe a better approach would be to have an enum of possible NoValue tuples. In your case that would be having some pre-generated states like State.Select which has id='' and name="-- Select --". And if the user really really wants to have a custom message just for that one select, then he can just define an empty State on the spot.

I don't get this last suggestion

Well I thought you could have defined defined states like:

DropList.BlankState = {
    Select: _TRL("-- Select --"),
    NoFilter: _TRL("-- No Filter --"),
    ...
}
// and then reference them like addState("", DropList.BlankState.Select)

Using a builder pattern might then be used to make use case simple.

let ctrl = Controls.DropList().addBlankState();
control.addState = function(name) {
    // TODO: check if blank State was all-ready added
     if (!name)
         name = DropList.BlankState.Select;
     control.addState("", name);
     return control;
};

Note the return control. That makes it chain-able, useful trick ;)

MichaelVyverman commented 3 years ago

@goranvinterhalter @MaartenWillemsAgilent based on your suggestions I think you want me to do the following:

  1. rename parameter to required (default to true)
  2. if required is set to True, it will add the blank state only specific cases the initial value is not set (set to '') and there are multiple choices
  3. if required is set to False, it will add the blank state always (unless the user specified another state in the choices with the same ID)

Another option is to go through the code and modify the current patterns everywhere where the blank option is not a valid option in the list of choices. I had a look and it is not as bad: only 16 instances 😄

ctrl = DropList({initialValue: theValue});
ctrl.addState('', _TRL('-- Select --'));
choices.forEach((choice) => {
    ctrl.addState(choice.id, choice.name);
});

to something like

ctrl = DropList({initialValue: theValue});
if(choices.length > 1 && !choices.includes('')){
    ctrl.addState('', _TRL('-- Select --'));
}
choices.forEach((choice) => {
    ctrl.addState(choice.id, choice.name);
});

And second pattern:

ctrl.clearStates();
ctrl.addState('', _TRL('-- Select --'));
newChoices.forEach((choice) => {
    ctrl.addState(choice.id, choice.name);
});

to something like

ctrl.clearStates();
if(newChoices.length > 1 && !newChoices.includes(ctrl.getValue())){
    ctrl.addState('', _TRL('-- Select --'));
}
newChoices.forEach((choice) => {
    ctrl.addState(choice.id, choice.name);
});

I can check the code again, but I see a reoccurring pattern that uses the ('', '-- Select myObject --') option instead of the general ('', '-- Select --'), so a parameter or a function or a wrapper or some other thing that reduces code duplication seems a good idea.

MichaelVyverman commented 3 years ago

True, your suggestion fixes the typo inconsistency issue! but I still need to perform the check if I need to add it or not based on the size of the choices. Based on the IDs of the blank states I could indeed engineer different behaviour (blank state for mandatories, blank states for filters, ...)

Edit: or keep the behaviour differences out of the control and do that in a wrapper control or something

goranvinterhalter commented 3 years ago

I was not really trying to solve the issue of hiding "-- Select --" If there is only one other choice... Maybe you can just ignore a blank if there is only one other option. That would then go into the part of the code that generates the js?

edit: but this is only for when you do not know how manny other options there would be. I'm actually unsure how useful hiding "-- Select --" is.

eidit2: Maybe the correct behavior is to hide "-- Select --" if the initial value was provided and it's not ""?

edit3: we are overthinking this :D

MaartenWillemsAgilent commented 3 years ago

I probably miss a lot of context so ignore my comment if it doesn't make sense :-), but I would assume that having a blank option also allows a field to be optional. Based on that I would not add it only in case the initial value of the dropdown is not part of the selected states.

Almost: having a blank option does not allow the field to be optional, but the reverse is true in 90% of the cases. Having a blank option can also mean the field is mandatory but no sensible value can be used as initial value.

I agree what I meant was that if you remove the '--Select--' option in certain scenarios, it becomes difficult for the user to go back to a 'no value' state once an item has been selected (if that should be possible off course but since it's a framework component I would keep the possibility. => https://stackblitz.com/edit/web-platform-ops96c?file=index.html

But on the other hand the '--Select--' option is actually a placeholder text instead of an actual option of the dropdown. I therefor like the approach most libraries follow nowadays.

But indeed lets not overthink this :-)

MichaelVyverman commented 3 years ago

I just came back to this conversation. We were indeed using the blank option both as an optional value and as a placeholder. We are not achieving the implementation of a true placeholder as the nice examples you showed.

MichaelVyverman commented 3 years ago

Drop in favour of Multiplicom/mastr_reporter#4865