Skip to content
This repository has been archived by the owner on Oct 19, 2021. It is now read-only.

fix(NumberInput): add button type and disabled #360

Merged
merged 3 commits into from
Dec 5, 2017
Merged

fix(NumberInput): add button type and disabled #360

merged 3 commits into from
Dec 5, 2017

Conversation

eddiemonge
Copy link
Contributor

  • Add button type to arrow button to prevent a form from being submitted
    when it is clicked

  • Disable the arrow buttons when the NumberInput is disabled just in
    case

Fixes #358

@@ -153,8 +153,13 @@ describe('NumberInput', () => {
);

const input = wrapper.find('input');
const upArrow = wrapper.find('.up-icon');
const downArrow = wrapper.find('.down-icon');
const upArrow = wrapper.find('.up-icon').parent();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

target the element that has the actual event handler

@@ -187,8 +192,8 @@ describe('NumberInput', () => {
);

const input = wrapper.find('input');
const upArrow = wrapper.find('.up-icon').last();
const downArrow = wrapper.find('.down-icon').last();
const upArrow = wrapper.find('Icon.up-icon').closest('button');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this would use parent as well but there is a bug in enzyme with mount that I opened

disabled
};

const createButton = dir => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored this so its dry'er

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do this if it's only at two call sites?

Copy link
Contributor Author

@eddiemonge eddiemonge Nov 28, 2017

Choose a reason for hiding this comment

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

1 < 2 so id say yes. I added 2 new lines of code and reduced the overall file by as much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm less concerned about file size and more concerned about closure allocs. I'm definitely down if the change made things much easier, but since it's so few call sites the change seems unnecessary.

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this! Only thing I'm cautious about is the DRY-ness added in for creating the buttons. Would love to hear your thoughts/reasoning behind it 😄

disabled
};

const createButton = dir => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do this if it's only at two call sites?

Copy link
Contributor

@marijohannessen marijohannessen left a comment

Choose a reason for hiding this comment

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

Looks good to me once comments are addressed! 👍 ✅

@tw15egan
Copy link
Collaborator

Can we also bump the version of carbon-components to 8.1.11? That way the disabled styles are bundled in. Otherwise, looks good 👍 ✅

@eddiemonge
Copy link
Contributor Author

the peer is set to "carbon-components": "^8.0.0", which would update to that version. updated

- Add button type to arrow button to prevent a form from being submitted
when it is clicked

- Disable the arrow buttons when the NumberInput is disabled just in
case

Fixes #358
@marijohannessen marijohannessen merged commit 028a209 into carbon-design-system:master Dec 5, 2017
@eddiemonge eddiemonge deleted the fix-358 branch December 11, 2017 22:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants