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.

Sign up for GitHub

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

Jump to bottom

FormToggle: component styles for external use #12385

Merged
merged 2 commits into from
Feb 6, 2019
Merged

FormToggle: component styles for external use #12385

merged 2 commits into from
Feb 6, 2019

Conversation

jrtashjian
Copy link
Contributor

@jrtashjian jrtashjian commented Nov 28, 2018 •
edited by gziolo
Loading

Description

Identified styles causing rendering issues with the FormToggle component outside of the block editor.

How has this been tested?

  1. Created a new plugin utilizing React and the @wordpress/components package.
  2. Imported and rendered a FormToggle component as well as a ToggleControl component.
  3. Verified component looks as issue FormToggle: Component wrongly styled #8871 has described (and shown with images).
  4. Verified component looks correct within the block editor.
  5. Applied CSS fixes to component.
  6. Verified component looks correct and unchanged within the block editor.
  7. Verified component looks correct in the test plugin (outside of the block editor).

Screenshots

Broken:

formtoggle-broken

Fixed:

formtoggle-fixed

Types of changes

Fixes #8871

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@jrtashjian
FormToggle wrapper class needs inline-block styling.
0085367 
Necessary for proper positioning of elements within .components-form-toggle.
@jrtashjian
Elements within FormToggle component need to have border-box box-sizing.
f8ce5ff 
This styling occurs within the block-editor on the post edit screen but does not exist outside of it. Moving this box-sizing value to the component resolves #8871.
@ajitbohra ajitbohra requested a review from jasmussen November 29, 2018 16:41
@ajitbohra ajitbohra added the [Package] Components /packages/components label Nov 29, 2018
@jasmussen jasmussen requested a review from a team November 29, 2018 17:05
@simison
Copy link
Member

simison commented Dec 6, 2018

Does it still work also when the element is disabled? (disabled state had some styling issues previously)

@mtias mtias added this to the WordPress 5.0.x Follow Ups milestone Dec 6, 2018
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Dec 14, 2018
@ajitbohra
Copy link
Member

Toggle works fine in the plugin, but not rending as expected outside editor

To test FormToggle and ToggleControl in isolation check
https://wpcalypso.wordpress.com/devdocs/gutenberg-components
https://wp-storybook.netlify.com/

@gziolo gziolo modified the milestones: WordPress 5.x, 5.0 (Gutenberg) Jan 25, 2019
@gziolo gziolo added Needs Design Feedback Needs general design feedback. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Jan 25, 2019
@gziolo gziolo requested a review from kjellr February 5, 2019 15:20
@gziolo
Copy link
Member

gziolo commented Feb 5, 2019

@jasmussen and @kjellr - can you help to land this PR as it seems like @jrtashjian doesn't work on it anymore.

@jrtashjian
Copy link
Contributor Author

@simison Yes, the existing disabled styles work when the element is disabled.

@ajitbohra Are you indicating the pull request doesn't fix the issue?

@gziolo sorry, haven't had a chance to get back on this one in a while. I think it's ready for a review and hopefully be merged in if ready!

@jasmussen
Copy link
Contributor

Thank you for this PR. I just took it for a spin, and honestly, it seems perfect to me as is, and no other change should be made IMO.

To answer @simison, if the form toggle gets wrapped in the components-disabled wrapper, it looks like this:

screenshot 2019-02-06 at 10 02 24

To answer @ajitbohra, this is how it looks like on storybook:

storybook

This is how it looks like on the calypso devdocs:

no margin change

So you can see it's offset. But this is CSS bleed that we, in my opinion, should not accommodate in this PR, but report as a bug in Calypso. Behold this wonderful that is inherited from Calypso:

screenshot 2019-02-06 at 09 58 48

If we uncheck that margin ...

screenshot 2019-02-06 at 10 08 58

We get this:

toggle

That margin is pretty broad and heavy handed, so it should be fixed in Calypso.

👍 👍 to this PR as is!

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

@jrtashjian congrats on your first code contribution to Gutenberg and thank you, everyone, for help with reviews 💯

@gziolo gziolo merged commit 7c33661 into WordPress:master Feb 6, 2019
@jrtashjian
Copy link
Contributor Author

Awesome. Thanks everyone! 🎉

@ajitbohra
Copy link
Member

@jrtashjian thanks for your contribution to Gutenberg.

Shared those links for reviewers to check component in isolation.

This was referenced Feb 18, 2019
Merged
Merged
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@jrtashjian @youknowriad
FormToggle: component styles for external use (#12385)
cd43dcf 
* FormToggle wrapper class needs inline-block styling.

Necessary for proper positioning of elements within .components-form-toggle.

* Elements within FormToggle component need to have border-box box-sizing.

This styling occurs within the block-editor on the post edit screen but does not exist outside of it. Moving this box-sizing value to the component resolves #8871.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
@jrtashjian @youknowriad
FormToggle: component styles for external use (#12385)
3052f9e 
* FormToggle wrapper class needs inline-block styling.

Necessary for proper positioning of elements within .components-form-toggle.

* Elements within FormToggle component need to have border-box box-sizing.

This styling occurs within the block-editor on the post edit screen but does not exist outside of it. Moving this box-sizing value to the component resolves #8871.
This was referenced Apr 30, 2020
Closed
Closed
Merged
Merged
Merged
Closed
Closed
Closed
Closed
Merged
Closed
Merged
Closed
Merged
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo approved these changes

@jasmussen jasmussen Awaiting requested review from jasmussen

@kjellr kjellr Awaiting requested review from kjellr

Assignees
No one assigned
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
5.1 (Gutenberg)
Development

Successfully merging this pull request may close these issues.

FormToggle: Component wrongly styled
7 participants
@jrtashjian @simison @ajitbohra @gziolo @jasmussen @youknowriad @mtias