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

Numeric Input Issues #618

Closed
3 tasks done
scottfr opened this issue Feb 4, 2017 · 11 comments
Closed
3 tasks done

Numeric Input Issues #618

scottfr opened this issue Feb 4, 2017 · 11 comments

Comments

@scottfr
Copy link

scottfr commented Feb 4, 2017

Thanks for the new component! Three issues encountered with it:

  • 1. It doesn't seem possible to set the size of the input using something like style={{width: '100%'}}

  • 2. I'm experience artifacts where the stepper is misaligned. See the following screenshot:

image

  • 3. It would be nice to be able to prevent the user entering non-numeric values like does
@giladgray
Copy link
Contributor

giladgray commented Feb 4, 2017

@scottfr thanks for trying out the new component so quickly!

  1. correct, it does not seem possible to make the component full-width. however, if you set px values then it works as expected.
    while some of our Blueprint components accept style, none of them truly support it; we much prefer styling via className.

  2. would you mind sharing the markup for this? never seen such a thing in our own testing.

  3. 👍

@scottfr
Copy link
Author

scottfr commented Feb 4, 2017

Here's the markup from within a dialog:

	<div class="pt-dialog-body">
		<label class="pt-label"><!-- react-text: 17 -->Width<!-- /react-text --></label>
		<div class="pt-numeric-input pt-control-group">
			<div class="pt-input-group">
				<label class="pt-label"><input type="text" min="1" placeholder="25" value="" class="pt-input" style="padding-right: 0px;"></label>
			</div>
			<div class="pt-button-group pt-vertical"></div>
		</div>
	</div>

@cmslewis
Copy link
Contributor

cmslewis commented Feb 4, 2017

Hi @scottfr - indeed, thanks for the quick feedback! I played with with your markup. The missing .pt-buttons in the button group notwithstanding, it look like the <label> wrapper around the <input> is the culprit. Did you add that manually, or was that added through some automatic/under-the-hood means?

EDIT: Here's my repro after adding a <label> around the <input>:
image

@scottfr
Copy link
Author

scottfr commented Feb 4, 2017

I add the label myself. I'm following the design illustrated here:

http://blueprintjs.com/docs/#components.forms.label

My code is as follows

<label className="pt-label" key={i}>
    {field.type!=='boolean'?field.name:''}  <-- Label name
    {this.fieldInput(field, i)}  <-- NumericInput Component
</label>

@giladgray giladgray added this to the 1.9.0 milestone Feb 6, 2017
@cmslewis
Copy link
Contributor

cmslewis commented Feb 6, 2017

Hi @scottfr,

Aha! There are two points of weirdness here, one of which is definitely a bug.

First, the bug: looks like there is indeed a CSS rule applying top-margins to .pt-inputs that live inside of .pt-labels. This breaks the layout as you described above, since the same top-margin is not applied to the button group. Perhaps the better solution here is to determine some way to declare a top-margin on whatever immediate child of the label might be present. I'll work on that and open a PR this week.

Second, a point of clarification: the NumericInput component is meant to function as a self-contained form field. The fact that there's an <input> inside of it is just an implementation detail that users like you shouldn't need to know or worry about. Concretely, this means you really just need an outermost <label>, not an additional one around the <input> child, as follows:

<div class="pt-dialog-body">
  <label class="pt-label">
    <!-- react-text: 17 -->Width<!-- /react-text --></label>
    <div class="pt-numeric-input pt-control-group">
      <div class="pt-input-group">
        <input type="text" min="1" placeholder="25" value="" class="pt-input" style="padding-right: 0px;">
      </div>
      <div class="pt-button-group pt-vertical"></div>
    </div>
  </label>
</div>

@cmslewis
Copy link
Contributor

cmslewis commented Feb 6, 2017

@scottfr Just chatted with some other Blueprint devs. Looks like the label-alignment issues are actually more pervasive: at present, any .pt-control-group inside of .pt-label will have this same problem (note that NumericInput is implemented as a .pt-control-group).

@leebyp's work in #594 actually addresses these issues for free. The only change will be to use the new pt-form-group markup instead of the existing pt-label markup for pt-control-groups once that PR merges.

So we'll wait for #594 to merge later this week, then we'll update the label/form-group documentation to clarify which to use for which form controls until we figure out a cleaner approach.

@cmslewis
Copy link
Contributor

cmslewis commented Feb 6, 2017

@scottfr Regarding your other points:

  1. Full-width: This will be fixed shortly by our upcoming solution to Feature: stretcher to fill remaining space for Control Groups (contains solution) #620. Thanks for filing!
  2. Numbers only: Definitely hear you that this is valuable behavior for NumericInput, but (1) in the interest of keeping the props API minimal and (2) given the ambiguity around handling negative signs, commas, decimal points, and other "number-ish" characters; the best approach is to implement this with a few lines of JS using the onKeyDown callback:
class MyComponent extends React.Component<{}, {}> {

    public render() {
        return (
            <NumericInput onKeyDown={this.handleKeyDown} />
        );
    }
 
    private handleKeyDown = (e: React.FormEvent<HTMLInputElement>) {
        if (!MyUtils.isKeyCodeForNumericCharacter(e.keyCode)) {
          // ignore the keystroke
          e.preventDefault();
        }
    }
}

@cmslewis
Copy link
Contributor

Hi @scottfr,

I thought more about this. I think I have a good way forward on restricting entry to number-ish characters, mirroring the functionality of the native input[type="number"]. The plan is to add a boolean prop tentatively called allowNumericCharactersOnly that, if enabled, supports only floating-point number characters as specified in the input[type="number"] spec:

A floating-point number consists of the following parts, in exactly the following order:

  1. Optionally, the first character may be a "-" character.
  2. One or more characters in the range "0—9".
  3. Optionally, the following parts, in exactly the following order:
    1. a "." character
    2. one or more characters in the range "0—9"
  4. Optionally, the following parts, in exactly the following order:
    1. a "e" character or "E" character
    2. optionally, a "-" character or "+" characters
    3. One or more characters in the range "0—9".

Distilling this down, we achieve the following character set:

  • -
  • +
  • 0–9
  • .
  • e
  • E

Some subtleties: note that the inputted number's structure is not validated in native input[type="number"]s. However, incrementing an invalid input will set the input value to 1 (slightly different from our current behavior, which simply clears the field):

2017-02-10 12 44 39

@scottfr
Copy link
Author

scottfr commented Feb 11, 2017 via email

@cmslewis
Copy link
Contributor

cmslewis commented Mar 6, 2017

@scottfr - we've got a freshly baked fix for full-width numeric inputs! See PR #792. Once that merges, all you need to do is <NumericInput className="pt-fill" />.

@scottfr
Copy link
Author

scottfr commented Mar 7, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants