-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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] Add "selectAllOnFocus" and "selectAllOnIncrement" props #705
Conversation
- B/c doesn't work in IE11, and - Test doesn't pass in FF
@@ -107,6 +119,8 @@ export class NumericInput extends AbstractComponent<HTMLInputProps & INumericInp | |||
buttonPosition: Position.RIGHT, | |||
majorStepSize: 10, | |||
minorStepSize: 0.1, | |||
selectAllOnFocus: false, | |||
selectAllOnIncrement: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why default these features off? they use to be enabled all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following EditableText
's example when I copied the prop description over, but I do prefer having them on. Will switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, both EditableText
and TimePicker
expose a selectAllOnFocus
prop, and both default the prop to false
. I'd prefer to stay consistent with their APIs by defaulting NumericInput
's selectAllOn*
props to false
too. Thoughts @giladgray?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on consistency, therefore I think it should be false
. If necessary, follow up with a PR that changes this default behavior to true
across the board
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works for me
Fixes #703 · Fixes #704
Checklist
NumericInput
's props tablesChanges proposed in this pull request:
selectAllOnFocus
prop.selectAllOnIncrement
prop.karma-unit-core
test in FF)Reviewers should focus on:
EditableText
, the unit tests I added forselectAllOnFocus
don't work in Phantom. Thus, the tests are included but disabled. Is there a way to get them working? If not, should I keep them or delete them?Screenshot