feat: add HasInputValue interface #3524
Conversation
yuriy-fix
force-pushed
the
feat/has-input-value
branch
from
d48f06e
to
807f80b
Compare
yuriy-fix
force-pushed
the
feat/has-input-value
branch
from
807f80b
to
24c2e1c
Compare
| * <code>false</code> otherwise | ||
| */ | ||
| @Synchronize(property = "_hasInputValue", value = "has-input-value-changed") | ||
| default boolean isInputValuePopulated() { |
There was a problem hiding this comment.
Should it be also renamed to align with the property name?
There was a problem hiding this comment.
Renamed to isInputValue 👌
|
Kudos, SonarCloud Quality Gate passed!
|
| * <code>false</code> otherwise | ||
| */ | ||
| @Synchronize(property = "_hasInputValue", value = "has-input-value-changed") | ||
| default boolean isInputValue() { |
There was a problem hiding this comment.
The name feels a bit odd to read, IMO. Could we change it to hasInputValue?
| default boolean isInputValue() { | |
| default boolean hasInputValue() { |
| * | ||
| * @author Vaadin Ltd | ||
| */ | ||
| public interface HasInputValue extends HasElement { |
There was a problem hiding this comment.
I'm normally a big fan of interfaces and re-usability but I have the feeling that this (as stated in the web component) is a private API and should not be exposed as public methods and therefore not be an interface, instead I would suggest to add this method inside all components as protected or package (private) API so that flow users aren't bothered with it.
The name hasInputValue or isInputValue is so generic that a lot of people probably going to use it just to check if there is something inside the field, expecting that setValue("something") changes it to true even tho the web component states "input element has a non-empty value entered by the user".
There was a problem hiding this comment.
I have had similar fears.
Hoped that not having specified parameter type (unlike HasValue), documentation mentioned that it's Intended for internal use and not having a setter would discourage users from using this interface instead of the proper one. Initial naming was isInputValuePopulated which is probably more descriptive name in terms of what it is returning.
However, I would agree that it still would be available for the users and somebody could probably use it by mistake.
I do appreciate your feedback as an actual user of our product! 🙇 I will change the approach in the PR in order to avoid confusions.
There was a problem hiding this comment.
I would argue that for most people any Java API is public that they can access, even tho there is Javadocs stating otherwise (which most people probably don't read)
Like people typing "textField.value" in their IDE and use the first method that pop ups and works.. with this in mind moving this somewhere else sounds the best to reduce public API cluttering and improve DX.
Another option could be to rename it so something totally technical like "isWebComponentPopulatedByUser" which sounds so obscure that probably less people are going to find (without input or value in it's name) and even use it.. but it perfectly describes it's meaning ;) But that feels like a way of "improved DX by obscurity" 😅
Edit: here is a perfect example why javadoc is "bad" from the weekend vaadin/flow#14240 (comment)
|
Closing in favour of introducing private methods in individual components. Example: #3550 |
Introduces
HasInputValuewhich can be implemented by a component to get the populated state of the internal input's value.Requires vaadin/web-components#4276
Related to vaadin/platform#3066