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.

Sign up for GitHub

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

Jump to bottom

Remove listener mutating text-field value #269

Merged
merged 5 commits into from
Jan 8, 2019
Merged

Remove listener mutating text-field value #269

merged 5 commits into from
Jan 8, 2019

Conversation

vladucu
Copy link
Member

@vladucu vladucu commented Dec 20, 2018 •
edited

Diff better viewed here

@vladucu vladucu added the bug Something isn't working label Dec 20, 2018
@vladucu vladucu self-assigned this Dec 20, 2018
@vladucu vladucu requested a review from tomnez December 20, 2018 16:39
vladucu
vladucu commented Dec 20, 2018
View reviewed changes
addon/components/polaris-text-field.js
// Making sure the new value has the same length of decimal places as the
// step / value has.
let decimalPlaces = Math.max(dpl(numericValue), dpl(step));
let newValue = Math.min(max, Math.max(numericValue + steps * step, min));
Copy link
Member Author

Choose a reason for hiding this comment

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

not really sure why linter changed these

tomnez
tomnez reviewed Dec 20, 2018
View reviewed changes
Copy link
Contributor

@tomnez tomnez left a comment

Choose a reason for hiding this comment

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

Should we change this to call onChange as the user types, rather than remove it altogether?

I played around with Polaris TextField in the sandbox and it calls onChange as the input value changes, regardless of focus/bur.

@vladucu
Copy link
Member Author

vladucu commented Jan 7, 2019

@tomnez I think I've found the root cause for this not working as expected...we've been using onchange instead of oninput event....can you give this a try please? (I've tested with the dummy app worked well)

tomnez
tomnez approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@tomnez tomnez left a comment

Choose a reason for hiding this comment

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

Tested this in an app and looks good 👍

@vladucu vladucu merged commit 09c6436 into master Jan 8, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/text-field-mutation branch January 8, 2019 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomnez tomnez tomnez approved these changes

Assignees

@vladucu vladucu

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vladucu @tomnez