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

Use the classnames library in TextWidget #315

Merged
merged 8 commits into from Nov 13, 2018

Conversation

cvalarida
Copy link
Contributor

Closes #314.

Like it says on the tin, this uses the classnames library in the TextWidget for the widgetClassNames option.

Types of changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've reviewed the guidelines for contributing to this repository.
  • I've checked style and lint with npm run lint.
  • I built the production version with npm run build.
    • Note: Nothing got updated in lib/, so I'm not sure what to do about that... 😕
  • I added tests to verify a bug fix or new functionality.
  • All tests pass locally with npm test.
  • My change requires a change to the documentation, and I've updated the documentation accordingly.

Copy link
Contributor

@dmethvin-gov dmethvin-gov left a comment

Choose a reason for hiding this comment

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

Great hat trick of code, docs, and tests. Just a few changes.

@@ -7,14 +8,15 @@ export default function TextWidget(props) {
if (!inputType) {
inputType = numberTypes.has(props.schema.type) ? 'number' : props.type;
}
const widgetClasses = classnames(props.options.widgetClassNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be done inline below, without creating a variable. It probably doesn't matter as far as performance is concerned but I have a slight preference for putting it below unless we need widgetClasses in two places.

@@ -84,4 +84,54 @@ describe('Schemaform <TextWidget>', () => {
tree.subTree('input').props.onBlur();
expect(onBlur.calledWith('1')).to.be.true;
});
it('should accept a string widgetClassNames prop', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the tests!

@@ -184,10 +184,14 @@ The us-forms-system code includes additional `uiSchema` functionality not found
'value': <p>Some text</p>
},

// A string of class names that are added to the widget for the current field.
// A list of class names that are added to the widget for the current field.
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the docs update! Since the documentation on this option applies to several widgets I think they all should be changed for consistency. The other references are in CurrencyWidget, SelectWidget, and ArrayCountWidget. Can you update those as well? I'm fine with just having the existing test on TestWidget for class names rather than duplicating it for the other widgets.

@cvalarida
Copy link
Contributor Author

@dmethvin-gov Thanks for the review! I made the changes you requested. Mind taking another look?

I'd normally use GitHub's UI to request another review, but it turns out I can't. 🤷‍♂️
image

Copy link
Contributor

@dmethvin-gov dmethvin-gov left a comment

Choose a reason for hiding this comment

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

Looks great!

@dmethvin-gov dmethvin-gov merged commit 6bc6294 into usds:master Nov 13, 2018
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.

widgetClassNames array doesn't split the class names appropriately
2 participants