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

Destructure css and classname to Checkbox/Radio wrapper components #55

Merged
merged 7 commits into from
Dec 11, 2019

Conversation

xdrdak
Copy link
Contributor

@xdrdak xdrdak commented Nov 28, 2019

Description

CSS prop and className prop was not getting forwarded to the right component.

This PR will now ship the right props to the right components.

How to test?

  • Checkout branch, run yarn dev
  • Open Storybook
  • Or check the deploy preview on Netlify (link available in comments)

Checklist

@netlify
Copy link

netlify bot commented Nov 28, 2019

Deploy preview for lightspeed-flame ready!

Built with commit 3c4820d

https://deploy-preview-55--lightspeed-flame.netlify.com

@xdrdak xdrdak marked this pull request as ready for review December 6, 2019 13:56
@xdrdak xdrdak requested a review from a team as a code owner December 6, 2019 13:56
display: -webkit-inline-box;
display: -webkit-inline-flex;
display: -ms-inline-flexbox;
display: inline-flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why has this changed to inline-flex?

@@ -29,7 +29,7 @@ const FormHelper: React.FC<FormHelperProps> = ({ status, children, ...restProps

export type BaseLabelProps = React.HTMLAttributes<HTMLLabelElement> & TextProps & { css?: any };
const BaseLabel = styled(Text)<BaseLabelProps>`
display: flex;
display: inline-flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, because of here. But why was this done as part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Labels should be inlined, not block by default.

This is to match up with regular styling.


.zIndex-2 {
z-index: 2;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

😄

@glambert
Copy link
Collaborator

glambert commented Dec 9, 2019

Also should we rename the PR subject to include only Radio and Checkbox?

@xdrdak xdrdak changed the title Destructure css and classname to wrapper components Destructure css and classname to Checkbox/Radio wrapper components Dec 9, 2019
@glambert
Copy link
Collaborator

glambert commented Dec 9, 2019

I've added comments on the Percy build regarding visual changes (all related to display: inline-flex): https://percy.io/Lightspeed/flame/builds/3416999.

@glambert
Copy link
Collaborator

I would actually scope out the display: inline-flex change and do that in a different PR. Let me know what you think @xdrdak.

@xdrdak
Copy link
Contributor Author

xdrdak commented Dec 10, 2019

I'll remove the inline flex thing. causes too many regressions

@glambert glambert merged commit 3276fcc into master Dec 11, 2019
@glambert glambert deleted the fix-inputs-regressions branch December 11, 2019 16:54
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.

2 participants