Conversation
d4f1121 to
5cb3e48
Compare
|
Thanks @mskacelik |
5cb3e48 to
ff05fbe
Compare
| private final InputGroupItem plusButtonItem; | ||
| private final Button plusButton; | ||
|
|
||
| private ChangeHandler<NumberInput, Double> blurHandler; |
There was a problem hiding this comment.
I would change these into a list of handlers. Like in https://github.com/patternfly-java/patternfly-java/blob/main/components/src/main/java/org/patternfly/component/form/TextInput.java#L102
Or even better re-use and expose the already existing lists of keyup and change handlers of the TextInput component.
There was a problem hiding this comment.
Ok, will try to experiment with it.
One thing is that I implemented the onPlus/onMinus handlers bit differently - because they pretty much by default handle the default incrementing/decrementing by 1. So having a List of (plus/minus)Handlers might be bit confusing => if you create a custom increment by 10 in the final it will increment by 11.
I guess in theory, I can have something like a custom flag that removes the default handler (increment/decrement by 1) - but maybe that could be a bit confusing for the user. What do you think?
There was a problem hiding this comment.
Yes, you're right. I think the built-in default should be hard-coded and should not be configurable. Then we could have a list of handlers that can be used to get notified if the user clicks on the +/- buttons.
There was a problem hiding this comment.
Not really sure If I understood the comment properly, can you elaborate a bit?
There was a problem hiding this comment.
Oh, I think I understand, so we will have List of onPlus/onMinus handlers and by default (the incrementing/decrementing by one) should be the first and cannot be removed.
All the other custom handlers should be appended to the List and will work like a pipeline?
So the custom incrementing by 10 is expected to be 11 (in a pipeline's context).
There was a problem hiding this comment.
What if we add a builder method like
public NumberInput step(int step) {
this.step = step;
return this;
}and then use NumberInput.step (which is one by default) in the built-in increase/decrease handlers.
There was a problem hiding this comment.
that could work ;)
There was a problem hiding this comment.
Actually, I did not go for the int step, but rather a UnaryOperator<Double> i.e.:
public NumberInput plusOperation(UnaryOperator<Double> operation) {
this.plusOperation = Objects.requireNonNull(operation, "operation must not be null");
return this;
}
public NumberInput minusOperation(UnaryOperator<Double> operation) {
this.minusOperation = Objects.requireNonNull(operation, "operation must not be null");
return this;
}
public NumberInput operations(UnaryOperator<Double> plusOperation,
UnaryOperator<Double> minusOperation) {
return plusOperation(plusOperation).minusOperation(minusOperation);
}Used like:
numberInput(90).operations(v -> v + 3, v -> v - 3))I think that this is even more flexible (allowing for doubling,dividing etc, having different operations for plus/minus).
There was a problem hiding this comment.
That's a great idea 👍
The only minor change I would make is to change the signature of the operations method to
public NumberInput operations(UnaryOperator<Double> minusOperation,
UnaryOperator<Double> plusOperation) {
return plusOperation(plusOperation).minusOperation(minusOperation);
}so that minus is before plus.
There was a problem hiding this comment.
With that in place we could add a step method that delegates to the operations method:
public NumberInput step(double step) {
return operations(v -> v - step, v -> v + step);
}|
|
||
| // ------------------------------------------------------ add | ||
|
|
||
| public NumberInput addUnit(String value) { |
There was a problem hiding this comment.
unit feels more like a builder method to me. Though there's something added to the number input, it's different to other add methods. Like there's no (sub)component added. So I would rename the methods to unit(...) and move them to the builder block.
|
|
||
| // ------------------------------------------------------ events | ||
|
|
||
| public NumberInput onBlur(ChangeHandler<NumberInput, Double> blurHandler) { |
There was a problem hiding this comment.
Like in
the handler would just be added to the list.| textInputItem = InputGroupItem.inputGroupItem(); | ||
| textInput = TextInput.textInput(TextInputType.number, Id.unique(), | ||
| String.valueOf(initialValue)); | ||
| textInput.on(change, this::handleInputChange); |
There was a problem hiding this comment.
The textInput.on() methods aren't what you're after. They register the handler on the TextInput.element() element, which is a <span/> element. You should use TextInput.input() to register the handlers instead. This method returns the underlying <input/> element.
There was a problem hiding this comment.
that did the trick with blur
ff05fbe to
4e323e2
Compare
3ed51ae to
3824e10
Compare
|
@hpehl CI seem to fail, but this "jbang" error was already present here: https://github.com/patternfly-java/patternfly-java/actions/runs/20955086586 So I guess it is not dependent on the PR. |
3824e10 to
4e3837d
Compare
|
@mskacelik Yeah, I saw this earlier on other jobs. Don't know why this fails from time to time. But no, it's not related to this PR. I triggered the job to run again. |
4e3837d to
76d1aea
Compare
hpehl
left a comment
There was a problem hiding this comment.
Thanks @mskacelik
Well done!
/cc @hpehl
closes #73
I am quite proud of this code, dare I say that I think its better than React's implementation.
With that said, I could not implement the blur event handler, for some reason it does not register (for the
TextInputfield).Could you please take a look at it? Thanks.
Tomorrow I will check whether the showcase's HTML code is exact the same as the React's one.