Skip to content
This repository has been archived by the owner on Oct 8, 2020. It is now read-only.

Create interfaces for methods common with DatePicker and TimePicker #6

Closed
vaadin-miki opened this issue Oct 24, 2019 · 4 comments
Closed

Comments

@vaadin-miki
Copy link

Setters and getters (and other methods) that are common to DateTimePicker and either of the two related components - DatePicker and TimePicker - should be in interfaces implemented also by those components.

The use case is that sometimes components are generated and properties of components are much easier to handle if they are defined in interfaces. Generating then take into consideration interfaces, without worrying how many components actually implement them.

An example of not having that functionality is in vaadin/flow#3241 - most components have a method setLabel, but there is no way to call it other than typecasting or reflection.

@Haprog
Copy link
Contributor

Haprog commented Jan 8, 2020

Did you have some specific setters/getters or methods in mind or an idea for any interface that is missing in addition to the HasLabel?

For the HasLabel I think the issue vaadin/flow#3241 needs to be fixed first so that Flow has the interface HasLabel before we can start using it in components.

Generically I don't think it's a good idea to add interfaces for everything possible "just in case" even if the same functionality would exist in 2 components (or more). I think it only makes sense when there is already a known valid use case for the interface.

For example DatePicker has setMin(LocalDate min) and LocalDate getMin() and similarly getMax. TimePicker has them too but with the type LocalTime. DateTimePicker will have them with LocalDateTime. Would it make sense to have an interface like HasMinMax<T> (T being the value type)? I'm not sure. At least I can't immediately think of why it would be useful.

Then there are things like initialPosition (LocalDate) and weekNumbersVisible" (boolean) which DateTimePicker will delegate to the DatePicker so there will be setters and getters for those in Java API of both DatePicker and DateTimePicker but I think it probably wouldn't make sense to add interfaces like HasInitialPosition and HasWeekNumbersVisible.

I quickly browsed through the Java API of DatePicker, TimePicker (and also TextField) and tried to think what new interfaces we could have. In addition to what I mentioned already I figured something like HasPlaceholder, HasRequired, HasRequiredIndicator, HasClearButton, HasHelperText, HasStep, HasLocale could be implemented, but I'm not sure if those make sense or not. Maybe one or few of them could be useful? But would like to see a use case for one before implementing.

Also this issue should probably be in some other repository since this is about changes to multiple components/repositories. I don't think we can create those interfaces in scope of this repository. Once we know more concretely which interfaces are needed for which components, issues should be opened about implementing those changes in the proper repositories.

@vaadin-miki
Copy link
Author

Agree with the generic interface for min and max not being useful. It would be better if the methods were named more descriptively and allow setting minimum date and time separately (use case: I want to restrict time selection to some hours, but no restrictions on date selection - I bet this will be frequently used), then setMinimumDate and setMinimumTime (and the like) should be in interfaces.

Side note: if the min/max setting is present also in other components, like number field, the method should probably be renamed to something more descriptive (setMinValue for example) and placed in an interface. Side side note: in context of time-related components setMin might be confused with setting minutes, if you are overly abbreviative ;)

As for methods in other components - some are covered with HasStyle, HasValue, some are not, and that is part of a different issue (generating components has not been accepted as a use case worth effort to make easier). No need to dwell into those right now, but in the long run we should make efforts to unify API this way (my personal opinion).

My point for this suggestion was to have an interface like HasTimeControls or TimeBasedComponent that just groups all methods shared between TimePicker and DateTimePicker, and similar for date. So, to clarify, an interface that groups functionally related methods - not interfaces for each setter/getter.

It probably takes little effort to introduce those two interfaces, and the number of times it will be used is likely marginal, but:

  • it does not disturb the usual workflow
  • it helps a lot in those cases where you work with component generation in one way or another
  • the trend should start somewhere, and creating a brand new UI component is probably best time and place for it.

Also, personally, I consider having such interfaces a good programming practice that helps in keeping API unified across projects. Use case: you might want to create a CustomField and just allow it to have time/date controls.

It may very well be that my thinking is just wrong and has no value, as I work with weird projects that are not quite representative. In such case, feel free to close the ticket :)

@Haprog
Copy link
Contributor

Haprog commented Jan 9, 2020

(use case: I want to restrict time selection to some hours, but no restrictions on date selection - I bet this will be frequently used

This would be a new feature that was not planned for the first version. But I agree it would be useful in some cases. Feel free to open a new feature request issue about it (in the web component repo as it should be first implemented for client side).

At the moment min and max define one continuous time range (as a pair of LocalDateTime) similar to how min and max define one continuous allowed range in DatePicker and TimePicker.

I think I kind of understand what you're getting at with the interfaces like HasTimeControls or TimeBasedComponent, but I'm still not sure if it makes sense in practice related to current implementations. Can you provide a concrete example of what exactly you would put into the TimeBasedComponent interface for example?

@Haprog
Copy link
Contributor

Haprog commented Jan 22, 2020

I'm closing this issue as too vague since I don't see any concrete use cases or concrete suggestions about the implementation that could be verified as working (if something would be implemented to close/fix this).

This may be reopened later if more information is given about what exactly should be done about this or if there's more info on exact use case(s) that could be verified for improvements. But if there is such a need, I think it would be even better to create a new issue with a more clear goal related to a specific problem or use case.

@Haprog Haprog closed this as completed Jan 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants