Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] feat(NumericInput): add leftElement prop #5383

Merged
merged 4 commits into from
Jun 16, 2022

Conversation

alex-c
Copy link
Contributor

@alex-c alex-c commented Jun 16, 2022

Fixes #5051

Hello, I created this draft PR, as I would like to fix #5051, but I have trouble building the project on Windows (see #4221) and am not sure what I should add in the ways of tests.

  • I added the necessary change and added an example to the docs. While I cannot run yarn verify (see above), yarn compile and yarn lint run fine. The change seems to work fine (see screenshot at the bottom).

  • I am also not sure what/how much I should do in the ways of tests. So far, there are no tests for NumericInput at all, as far as I can see... Nor are there for InputGroup?

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Add leftElement as an alternative to leftIcon for NumericInput, the same way it is for InputGroup. Since NumericInput uses InputGroup, the leftElement prop can just be passed down.

Reviewers should focus on:

leftElement should be working for NumericInput.

Screenshot

The following screenshot is from the example I added to the docs. Setting a button with a popover in leftIcon breaks the layout. With leftElement it works fine:

image

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @alex-c! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@alex-c alex-c changed the title Ac/left element for numeric input Add leftElement to NumericInput Jun 16, 2022
@alex-c alex-c changed the title Add leftElement to NumericInput [core] Add leftElement to NumericInput Jun 16, 2022
@adidahiya
Copy link
Contributor

adidahiya commented Jun 16, 2022

Sorry about the trouble building the project in Windows.

Generally I advise running the individual build scripts in each package for the best debugging experience. For example:

cd packages/core
yarn compile
yarn lint
yarn test

It looks like you've managed to trigger the CI build successfully, though, so you should get build & test results here within a few minutes of pushing your commits.

There are actually quite a few tests for NumericInput: https://github.com/palantir/blueprint/blob/develop/packages/core/test/controls/numericInputTests.tsx

I'll review this PR soon!

@alex-c
Copy link
Contributor Author

alex-c commented Jun 16, 2022

Oh, I guess I only looked for tests in the forms directory. I'll take another look and see if I can add some meaningful tests.

@alex-c
Copy link
Contributor Author

alex-c commented Jun 16, 2022

I have now added two basic test cases.

@adidahiya adidahiya marked this pull request as ready for review June 16, 2022 20:12
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

docs preview here

nice, this looks great! thanks for the PR, I appreciate the new option added to the NumericInput example in the docs.

@adidahiya adidahiya changed the title [core] Add leftElement to NumericInput [core] feat(NumericInput): add leftElement prop Jun 16, 2022
@adidahiya adidahiya merged commit 3e42d38 into palantir:develop Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow adding a leftElement to a NumericInput (Blueprintjs 4.0.0-beta.10)
3 participants