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

feat: add textClassName prop and unit test #1184

Merged
merged 6 commits into from
Sep 2, 2020

Conversation

Mike-Diaz
Copy link
Contributor

@Mike-Diaz Mike-Diaz commented Aug 25, 2020

Description

This adds the textClassName prop inside of Checkbox in order for custom classes to be applied to the element. This is necessary for styles to target the class name and be applied directly to the children. This does not change the way the component originally works since the standard class will always be applied.

Linked to PR: https://github.concur.com/coreui/nui-widgets-lab/pull/124

Added Unit tests

@netlify
Copy link

netlify bot commented Aug 25, 2020

Deploy preview for fundamental-react ready!

Built with commit 11c3c9f

https://deploy-preview-1184--fundamental-react.netlify.app

@Mike-Diaz Mike-Diaz requested a review from jbadan August 25, 2020 22:56
@Mike-Diaz Mike-Diaz self-assigned this Aug 25, 2020
@Mike-Diaz Mike-Diaz added the enhancement New feature or request label Aug 25, 2020
@Mike-Diaz Mike-Diaz requested a review from a team August 25, 2020 22:57
@@ -119,6 +119,16 @@ describe('<Checkbox />', () => {
).toContain('wonderful-styles');
});

test('should set validationClassName on the popover', () => {
Copy link
Contributor

@skvale skvale Aug 26, 2020

Choose a reason for hiding this comment

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

should set childrenClassName on the checkbox children

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 that is super weird. I was sure I changed the title of this test, thanks!

@@ -119,6 +125,8 @@ Please ensure you are either using a visible \`FormLabel\` or an \`aria-label\`
);
}
},
/** Additional classes to apply to children */
childrenClassName: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have to be a little more careful with the name here - if the child is not a string, it won't add this class. Do we want that? Do we not? I don't know...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought perhaps this was not the best name. Would you have a suggestion of a more semantic, or explicit name? Perhaps stringChildrenClassName?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think textClassName would be fine

@Mike-Diaz Mike-Diaz changed the title feat: add childrenClassName prop and unit test feat: add textClassName prop and unit test Sep 2, 2020
@Mike-Diaz Mike-Diaz merged commit b800bf9 into master Sep 2, 2020
@Mike-Diaz Mike-Diaz deleted the feat/checkbox-children-classnames branch September 2, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants