-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Add a checkbox to fixtures UI to choose React production build #13786
Add a checkbox to fixtures UI to choose React production build #13786
Conversation
@philipp-spiess kindly deployed this for taking a quick look: https://philipp-react-dom-fixtures.netlify.com/ |
Details of bundled changes.Comparing: e2e7cb9...8d0db37 scheduler
Generated by 🚫 dangerJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sending this in! I've requested a few small changes, but I think this is otherwise good to go.
Thanks for contributing!
onChange={this.handleProductionChange} | ||
/> | ||
<label htmlFor="react_production"> | ||
<span className="header__label">Production</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you eliminate this extra span by adding the class name to the label?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea.
type="checkbox" | ||
checked={this.state.production} | ||
onChange={this.handleProductionChange} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a style to center this vertically with the label? maybe:
.header__checkbox {
vertical-align: middle;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
@poeschko Hey! Just wanted to check in. I'd be happy to move this along if you'd like. |
Thanks for the comments (which I have addressed now), and sorry for the delay. |
Awesome, thank you! I'm going to do a quick build locally, but this is ready to 🚢! |
Checks out. Awesome stuff, thanks for doing this! |
Thanks for reviewing @nhunzaker 🎉 And congrats for getting this shipped @poeschko 🙌 |
…ook#13786) * Add a checkbox to fixtures UI to choose React production build * Assign header__label class name to label directly, instead of using a separate span * center the production checkbox vertically
…ook#13786) * Add a checkbox to fixtures UI to choose React production build * Assign header__label class name to label directly, instead of using a separate span * center the production checkbox vertically
This started out as a part of #13613 which will probably be closed.