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

[EuiFieldNumber] Added any as a step option #3562

Merged
merged 9 commits into from
Jun 10, 2020
Merged

Conversation

shrey
Copy link
Contributor

@shrey shrey commented Jun 4, 2020

Summary

Added input type "any" as an attribute of step of EuiFieldNumber.
Fixes #3550

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • [ ] Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos @chandlerprall
I've made the necessary changes, @cchaos learnt something new, thanks.

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@shrey
Copy link
Contributor Author

shrey commented Jun 5, 2020

@cchaos @chandlerprall Could you review this PR?

@snide
Copy link
Contributor

snide commented Jun 5, 2020

Can you please update your PR description to match the changes you've made. I know this one is pretty simple, but at least removing the boilerplate summary would help.

Thanks for helping out!

@shrey
Copy link
Contributor Author

shrey commented Jun 5, 2020

@snide Yeah, I actually made another PR which I later closed and forgot to write the description for this one.

@cchaos cchaos changed the title Added any as a step option [EuiFieldNumber] Added any as a step option Jun 5, 2020
@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2020

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3562/

Okay, I'll directly apply your suggestions

Co-authored-by: Caroline Horn <[email protected]>
@shrey
Copy link
Contributor Author

shrey commented Jun 5, 2020

@cchaos any other improvements?

@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3562/

@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2020

Hmmm, the prop is getting passed, but it seems to not actually be working. I can't type any decimals when step="any".

Screen Recording 2020-06-05 at 02 50 PM

@thompsongl
Copy link
Contributor

Hmmm, the prop is getting passed, but it seems to not actually be working. I can't type any decimals when step="any".

This is only an issue in the docs. The onChange fn alters the input:

const onChange = e => {
const sanitizedValue = parseInt(e.target.value, 10);
setValue(isNaN(sanitizedValue) ? '' : sanitizedValue);
};

We should update the docs, though

@shrey
Copy link
Contributor Author

shrey commented Jun 6, 2020

@cchaos Should I update some documentation?

@cchaos
Copy link
Contributor

cchaos commented Jun 8, 2020

@shrey Yes, let's go ahead and remove the sanitization

@shrey
Copy link
Contributor Author

shrey commented Jun 8, 2020

@cchaos So should we get rid of the onChange function all together?

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos @thompsongl I made the changes in the docs, could you review them?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Jenkins, test this

@@ -19,6 +18,7 @@ export default () => {
value={value}
onChange={e => onChange(e)}
aria-label="Use aria labels when no actual label is in use"
step={3}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add a step like this as it could confuse consumers that this is the default.

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 forgot to remove that, I was just testing it

@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2020

retest

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3562/

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos Could you review it now?

@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2020

retest

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3562/

@shrey
Copy link
Contributor Author

shrey commented Jun 10, 2020

@cchaos The tests passed :) any other changes?

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Awesome. Checked the docs example and adding step="any" works.

@cchaos
Copy link
Contributor

cchaos commented Jun 10, 2020

retest

@cchaos cchaos merged commit 639d665 into elastic:master Jun 10, 2020
phyolim pushed a commit to phyolim/eui that referenced this pull request Jun 11, 2020
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.

EuiFieldNumber should allow step attribute with "any" value
7 participants