Skip to content

Add NumberInput component#352

Merged
hpehl merged 1 commit intopatternfly-java:mainfrom
mskacelik:NumberInput
Jan 16, 2026
Merged

Add NumberInput component#352
hpehl merged 1 commit intopatternfly-java:mainfrom
mskacelik:NumberInput

Conversation

@mskacelik
Copy link
Contributor

/cc @hpehl
closes #73

I am quite proud of this code, dare I say that I think its better than React's implementation.

  • IMO better/clearer API => no need for complicated handlers - the ones that React's showcase has
  • does not contain bugs that the React one has.

With that said, I could not implement the blur event handler, for some reason it does not register (for the TextInput field).

// FIXME: blur event is not registered for some reason
this.blurHandler = (e, c, v) -> {
    logger.info("Blur event fired");
};

// ...
// does not register (unlike the change and keydown)
textInput.on(blur, this::fireBlurHandler);

// ...

private void fireBlurHandler(Event event) {
    blurHandler.onChange(event, this, value);
}

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.

@hpehl
Copy link
Contributor

hpehl commented Jan 13, 2026

Thanks @mskacelik
Great to see you back contributing. I'll review asap.

@hpehl hpehl self-requested a review January 14, 2026 17:29
private final InputGroupItem plusButtonItem;
private final Button plusButton;

private ChangeHandler<NumberInput, Double> blurHandler;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mskacelik mskacelik Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really sure If I understood the comment properly, can you elaborate a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that could work ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@hpehl hpehl Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like in

private final List<ChangeHandler<TextInput, String>> keyupChangeHandlers;
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that did the trick with blur

@mskacelik mskacelik requested a review from hpehl January 15, 2026 13:56
@mskacelik mskacelik force-pushed the NumberInput branch 3 times, most recently from 3ed51ae to 3824e10 Compare January 16, 2026 16:05
@mskacelik
Copy link
Contributor Author

@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.

@hpehl
Copy link
Contributor

hpehl commented Jan 16, 2026

@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.

Copy link
Contributor

@hpehl hpehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mskacelik
Well done!

@hpehl hpehl merged commit cba6811 into patternfly-java:main Jan 16, 2026
1 check failed
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

Successfully merging this pull request may close these issues.

Implement number input component

2 participants