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

Should we simplify form/selector components? #318

Open
antonymilne opened this issue Feb 19, 2024 · 6 comments
Open

Should we simplify form/selector components? #318

antonymilne opened this issue Feb 19, 2024 · 6 comments
Assignees

Comments

@antonymilne
Copy link
Contributor

antonymilne commented Feb 19, 2024

Background context

So far we have the following possible SelectorType, which can be used in Parameter.selector or Filter.selector:

  • Checklist, based on dcc.Checklist
  • Dropdown, based on dcc.Dropdown
  • RadioItems, based on dcc.RadioItems
  • RangeSlider, based on dcc.RangeSlider
  • Slider, based on dcc.Slider

Maybe, where suitable components are available, these will change to dbc components in the future when we are fully bootstrap compatible.

In due course we would like to enable a Form model that enables you to use these outside the context of a filter/parameter. I've already done this many times for custom apps by using add_new_type, but eventually it should be native functionality.

To this end, a _FormComponentType exists but is not yet public. This contains:

  • all the above SelectorType
  • UserInput, based on dbc.Input (private)
  • Textarea, based on dbc.Textarea (private; actually doesn't exist in _FormComponentType yet but should do)
  • Button, based on dbc.Button (public, since it's also part of ComponentType so that it can be used directly in Page.components or Container.components, like a Graph, Card, Table)

Remember also that we aim to keep things simple rather than supply every possible sort of component out of the box. We don't want to have too many models.

We are now considering adding a DatePicker in #309. This raises some questions that don't need to be resolved now but should be pondered over time...

Why two sliders?

From #309 (comment):

@antonymilne said:

I don't know why we have a separate RangeSlider and Slider at the moment 🤔 As far as I can tell RangeSlider with a single element value = [1] would function identically to a Slider. So we maybe should have simplified this to just a single vm.Slider model. Did we consider this before? I can't remember. @maxschulz-COL?

@AnnMarieW do you know if there's any difference in behaviour between RangeSlider with a single element value vs. Slider? Is there a reason there's two separate dcc components for these? Other than it's maybe easier for people to find the right component because people know to search for a slider vs. a range slider. From the vizro side, we generally try to aim for something as simple as possible rather than many possible options so maybe it would make sense for us to just provide a single Slider model that handles both. What do you think?

@AnnMarieW said:

Interesting question about using a single value in the Range Slider. It seems to work, but it would be possible to change it to a range slider when updating it with two values in a callback. So there would need to be some type of check to make sure that only a single value is returned. I'm not sure if there would be any other unintended issues.

Removing/renaming Slider/RangeSlider would be a breaking change, but we can definitely do it with suitable deprecation warnings etc.

Why UserInput and Textarea?

I didn't look into these properly yet, but at a glance they feel quite similar to me. Should they also be a single model? They're not public so we can merge/rename/whatever as we please.

How to handle DatePicker and DateRangePicker?

Will be resolved in #309.

Names

Our policy so far has been to name our models according to the underlying dash component, unless where that seems confusing (hence instead of Input we use UserInput).

  • Dash components already have well-established/considered names, so no need for us to think about it again
  • it's more familiar to Vizro users coming from Dash
  • it makes it clear what Dash component our model is based on, so easier for a user to find relevant docs on that component

I don't want to bikeshed and spend a lot time thinking again about names, but maybe we should consider whether this the right approach though. It sort of couples us to a particular Dash component which we might want to change, and makes it tricky in the edge case that a model could return different Dash components like we might do for Date(Range)Picker.

e.g. I kind of like the simplicity of naming in https://observablehq.com/documentation/inputs/ where it's just Date rather than DatePicker, although DatePicker is definitely also common terminology and well understood.

Should we go for more "declarative" names like Select or Selector rather than imperative ones like Dropdown? We considered this before and decided it made the UX more rather than less confusing, e.g. people are more likely to say "I want radio buttons here and checkboxes there" rather than "I want a selector that enables me to select multiple items.

Next steps

@antonymilne
Copy link
Contributor Author

Now to respond to @AnnMarieW here:

Interesting question about using a single value in the Range Slider. It seems to work, but it would be possible to change it to a range slider when updating it with two values in a callback. So there would need to be some type of check to make sure that only a single value is returned. I'm not sure if there would be any other unintended issues.

As far as this would be an issue, I believe it already exists because a RangeSlider that's intended to have two elements in values could already have a callback that sets more than two values, like values = [1, 2, 3]. This will likely break the filtering functionality but that's ok by me because it's an unlikely thing for someone to do unintentionally. The confusion between 1 vs. 2 values is definitely more likely to cause issues, but I am pretty confident they will not be big problems.

@huong-li-nguyen
Copy link
Contributor

huong-li-nguyen commented Feb 19, 2024

I'll respond in detail tomorrow since I want to ponder more on this. But my initial impression:

Generally, it makes sense to me for the DatePicker/DateRangePicker to be combined. I don't think we should do it for the UserInput and TextArea, though, as they have very different arguments and are semantically also different (HTML-wise). For the Slider / RangeSlider - I'm still trying to figure out. The current code seems like a nightmare to combine these two into one model 😅, because of all the extra text input callback logic and extra validations that might be required.

I think we initially didn't merge these because of user experience and we've had this unsaid rule of returning only one "main" dash component per model. On user experience: Even though you could select a single value with a RangeSlider, you still have to move two handles. Me, being a normally lazy user - if I want to change one value, I want to change one handle only 😄 I think the other reason was the text inputs in the sliders and their different client-side callbacks attached. Looking at the code of the RangeSlider - it already looks quite large. Also we were not sure what other functionalities the QB design library might add to the Slider / RangeSlider.

The DatePicker / DateRangePicker components have most of the arguments in common, and the ones they don't have in common we probably won't expose publicly anyways (except for the allowSingleDateInRange). For example, these are the only ones these two do not have in common:

Datepicker:

  • allowFreeInput (boolean; optional):

DateRangePicker:

  • allowSingleDateInRange (boolean; optional):
  • labelSeparator (string; optional):

For me, it makes sense to combine the components if the majority or even all of the arguments are the same. If there is a more significant deviation (as for UserInput and TextArea), I would keep them separate. The question I have is:

  • How will this affect the typing validations for the pydantic models(e.g., Slider requires a single value, RangeSlider a list)? How do we enforce the type hints to throw validation errors in time properly? Would we have to add more validators with if/else statements?
  • Which arguments do the two components we want to merge have in common? Which arguments deviate? Would we ever want to expose the ones deviating publicly? If yes, will this lead to a bunch of validators with if/else statements?
  • How do other tools do it? E.g. I see that streamlit has one Slider component (for slider, range slider, datepicker, daterangepicker), while they have separate components for input / textarea

@maxschulz-COL
Copy link
Contributor

Interesting discussion, @huong-li-nguyen already said most of the relevant points, so I'll try to be short:

On range_slider and slider

  • original reasons as far as I can remember: one dash component per model, complex callback architecture, legacy! (tbh I think we didn't think about it too deeply)
  • if I understand correctly: use the range_slider as slider by having both handles at the same position - strongly against this (for the reason Li I think mentioned)
  • if I understand correctly: use the range_slider as slider by some configuration option then returning different model (similar to what A proposes here: maybe but probably not (see below)

On the Datepicker and DateRangePicker

  • great comments on the other thread
  • picking a single date with a range picker - just as above strongly against (it's ugly and unituitive)
  • returning either model: maybe, but probably not (see below)

On naming and returning different Dash components in a single model

  • unspoken rule of providing one Dash component per model may have more advantages than we think
  • similar, naming similarly to Dash may have more advantages than we think
  • we already struggle with people realising that we are built on top of Dash, and e.g. how to look for component attributes in actions and callbacks
  • hence returning different Dash components based on configuration could lead to confusion
  • additionally, doing this automatically could cause actual stability issues (thinking of input and output attributes set in the model)
  • the naming differently from Dash probably aggravates this problem potentially

Why it still could make sense

  • Vizro aims to be above all declarative, and this would help with it
  • We always try to simplify Dash, and we may hit limits in doing so quickly if we restrain to one component per model

As for Forms

Agree with the next steps, and was keen to try and start this this sprint, but it is probably unrealistic that we get to that.

I have a few additional opinions/ideas about Forms, stemming more from experiences in the past and how users e.g. wanted to handle Kedro pipeline configurations and more complex forms that go beyond ~3 input parameters. Not sure if this should be in the first version, but definitely some learnings we should keep in mind.

@antonymilne
Copy link
Contributor Author

antonymilne commented Feb 20, 2024

Thanks for the comments both! Just to clarify, we should absolutely not replace the single-value Slider with a RangeSlider that makes you move both handles at once - that would just be horrible UX. Similarly for using a DateRangePicker component when all you actually want is one date. So the only options for these mind are:

  1. Separate models: RangeSlider and Slider; DateRangePicker and DatePicker
  2. One model that can return one of two underlying Dash components depending on how the model is invoked: Slider; DatePicker

We don't necessarily need to use the same solution for both cases, e.g. we could use option 1 for Slider and option 2 for DatePicker, although it might be nice to do so eventually for consistency if we think the same option makes sense in both cases.

At the moment the case for option 2 is stronger for DatePicker than it is for Slider, because it's simpler (no clientside callbacks etc.) and the component doesn't yet exist so doesn't mean breaking anything (as Slider currently follows option 1).

Let's leave Slider aside for now and just think about DatePicker. Here @maxschulz-COL says "maybe, but probably not". Given how similar the arguments are like @huong-li-nguyen pointed out my current opinion is "in an ideal world, probably yes, but given the current codebase, probably not". But I'm not sure... So let's discuss more in our call tomorrow anyway 🙂

@AnnMarieW do you have any thoughts on this? Just for the date picker, do you think we should simplify to one vm.DatePicker model that can produce either dmc.DateRangePicker or dmc.DatePicker? Or should we have two separate models vm.DatePicker and vm.DateRangePicker?


How will this affect the typing validations for the pydantic models(e.g., Slider requires a single value, RangeSlider a list)? How do we enforce the type hints to throw validation errors in time properly? Would we have to add more validators with if/else statements?

Not sure exactly, but if we had a single Slider model then e.g. if you input value = 1 then we could coerce it to value = [1] so it can be used in RangeSlider. I don't think validation would become hugely more complicated. It's probably the other bits of code where the complexity would be, e.g. filter need to know whether to treat something as one or two values.

Which arguments do the two components we want to merge have in common? Which arguments deviate? Would we ever want to expose the ones deviating publicly? If yes, will this lead to a bunch of validators with if/else statements?

Indeed, if this is yes then it leads to unpleasant complexity where some arguments are only relevant to some selectors, and I'd strongly prefer to avoid that. This was one of the original arguments against having a generic Selector component, since e.g. the multi argument would only be relevant for certain types of selector, and it's still a strong argument I think. So if RangeSlider and Slider have drastically different arguments then best to keep them separate.

@AnnMarieW
Copy link
Contributor

I know I don't have all the info, but it seems like separate models adds less complexity.

@antonymilne
Copy link
Contributor Author

Update: in #309 we went for a single DatePicker model which switches between vm.DatePicker and vm.DateRangePicker. @petar-qb noted that this is actually consistent with upcoming changes in future versions of dmc when they update to v6 or v7 of Mantine.

These versions expose a new argument type="default"/"range"/"multiple", where multiple behaves like a slider with multiple values selectable. When it comes to sliders, this is actually sort of already possible in vizro, but we don't (ever?) see people try to do it. So tbd if we care about supporting it for dates or sliders at any point in the future.

My current feeling is it would probably make sense to merge Slider and RangeSlider into a single model in due course.

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

5 participants