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

Resolves #625 - Add support for the className prop for the Column component #626

Merged
merged 2 commits into from
Sep 10, 2020

Conversation

nicklemmon
Copy link
Contributor

Resolves #625

What Changed

  • className accepted as a prop for the <Column/> component

How To Test or Verify

  • On the Storybook site, try:
<Box as={Column} display="none">I am invisible!</Box>

PR Checklist

  • Incorporate feedback

Copy link
Contributor

@jonambas jonambas 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, but added a suggestion

@@ -46,6 +47,7 @@ Column.displayName = 'Column';
Column.propTypes = {
children: PropTypes.node,
width: PropTypes.oneOfType([PropTypes.oneOf(['content']), PropTypes.number]),
className: PropTypes.string,
Copy link
Contributor

@jonambas jonambas Sep 9, 2020

Choose a reason for hiding this comment

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

I think this works. For your use case you could expose display

import { display } from 'styled-system`;
import { createPropTypes } from '@styled-system/prop-types';

...


const { width, children, className, display } = props;

...

<StyledColumn display={display} />

...

Column.propTypes = {
  ...createPropTypes(display.propNames),
}

Copy link
Contributor

Choose a reason for hiding this comment

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

styled(Box) already includes display so no need to add it to the StyledColumn declaration

Copy link
Contributor

@jonambas jonambas left a comment

Choose a reason for hiding this comment

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

LGTM

@jonambas jonambas merged commit 85d224b into main Sep 10, 2020
@jonambas jonambas deleted the resolve-#625 branch February 18, 2021 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Column component cannot be re-styled using <Box/> or styled-components
2 participants