vaadin / flow-components

Java counterpart of Vaadin Web Components
100 stars 66 forks source link

TabSheet does not delegate setAutoselect to Tabs #5764

Open timomeinen opened 11 months ago

timomeinen commented 11 months ago

Description

Documentation states that Tabs and TabSheet may have setAutoselect but this method is missing on TabSheet. It should provide this method and delegate to Tabs. Or alternativly, add a getTabs method in TabSheet to get access to the Tabs object.

Expected outcome

TabSheet.setAutoselect(false) should disable autoselect.

Minimal reproducible example

new TabSheet().setAutoselect(false);

Steps to reproduce

Method not available.

Environment

Vaadin version(s): 24.2

Browsers

No response

web-padawan commented 11 months ago

As mentioned in https://github.com/vaadin/flow-components/issues/5232#issuecomment-1700658159, we shouldn't add getTabs but instead consider adding missing API.

rolfsmeds commented 10 months ago

This is a strange API anyway, at least in terms of naming.

@timomeinen, could you describe how this is better than just setSelectedTab(null)?

timomeinen commented 10 months ago

@rolfsmeds I didn't know that passing a null would effectivly disable the autoselect. My intention was to get rid of the fired event. For a developer this "feature" is not clear. The documentation of Tabs component states that setAutoselect is available on both Tabs and TabSheet.

But more importantly, from an API perspective (or CleanCode) I would find it strange to pass in a null as a parameter or to tell it differently: give null a meaning in controlling behaviour. I would favour a API with good method names to control component's behaviour, like setAutoselect(false). Additionally, this method is already known from the Tabs component.

rolfsmeds commented 10 months ago

Thanks for the clarification @timomeinen. The reason I'm asking is that I'm skeptical that a separate method called setAutoselect is actually any more intuitive and discoverable than passing null to setSelectedTab, as null values are pretty much the de-facto way of passing a "none" value in Java.

In fact, if setSelectedTab(null) gets the job done, I would prefer to remove setAutoselect from Tabs, as there should always be very good reasons to bloat the API with two different ways of doing the same thing. At the very least, I would rename setAutoselect to something less strange.

timomeinen commented 10 months ago

Dear @rolfsmeds, Thank you for your interest. Personally, I can work with both solutions if they are clearly stated in the documentation, but I would favour to not pass any null values and instead use a dedicated method with a good name.

You as the owners and experts of the api needs to find a consistent way how the api is being used and documented.

But I do not agree, that passing null is the "de-facto way of passing a none". In the Clean-Code community it is more common to provide dedicated methods and passing or returning null is a code smell for an inadequate api. And I think in the last years, returning null has been replaced by Optional. You can find reasoning for these in the books "Clean Code" or "Efficient Java".

To sum up, I think two solutions might be:

  1. Update documentation about passing null to disable the feature of autoselect
  2. Enhance TabSheets with setAutoselect and delegate to Tabs api