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

Fix issue with disabled togglecontrol double border #12091

Merged
merged 2 commits into from Nov 20, 2018

Conversation

Projects
None yet
5 participants
@jasmussen
Contributor

jasmussen commented Nov 20, 2018

Fixes #12049.

This PR fixes an issue where a togglecontrol, wrapped in a Disabled component, would show a double border. The issue was that it inherited a border from the parent checkbox style (because the toggle is technically a reskinned checkbox).

Before:

screenshot 2018-11-20 at 10 04 48

After:

screenshot 2018-11-20 at 10 04 34

Fix issue with disabled togglecontrol double border
Fixes #12049.

This PR fixes an issue where a togglecontrol, wrapped in a Disabled component, would show a double border. The issue was that it inherited a border from the parent checkbox style (because the toggle is technically a reskinned checkbox).

@jasmussen jasmussen added this to the 4.6 milestone Nov 20, 2018

@jasmussen jasmussen self-assigned this Nov 20, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Nov 20, 2018

@sirreal

I didn't recompile styles, but I verified that a toggle control wrapped in Disabled does not have the double border when manually adding this style via browser dev tools:

.components-toggle-control .components-base-control__field .components-form-toggle__input {
    border: none;
}

Looks like this fixes the issue. Thanks!

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 20, 2018

Thanks for the review! Not sure why the button doesn't turn green with your approval. But in any case, it's milestoned, so we'll get it in.

@simison

Compiled and confirmed this fixes it for both content area and the sidebar:

image

@simison

This comment has been minimized.

Contributor

simison commented Nov 20, 2018

Not sure why the button doesn't turn green with your approval.

We don't have special rights to this repo. 😉

@simison

This comment has been minimized.

Contributor

simison commented Nov 20, 2018

Looks like checked state is still messed up even with this branch:

image

via Automattic/wp-calypso#28701

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 20, 2018

CC: @ockham, perhaps the real fix is to add a disabled style to the component itself?

@ockham

This comment has been minimized.

Contributor

ockham commented Nov 20, 2018

CC: @ockham, perhaps the real fix is to add a disabled style to the component itself?

Not sure I understand -- adding disabled styling was what I planned to do in the first place: #12105

@@ -10,4 +10,9 @@
display: block;
margin-bottom: 4px;
}

// This overrides a border style that is inherited from parent checkbox styles.
.components-form-toggle__input {

This comment has been minimized.

@ockham

ockham Nov 20, 2018

Contributor

Since this styles components-form-toggle__input -- should the styling actually go to packages/components/src/form-toggle/style.scss so it'd also apply to FormToggle when not wrapped in a ToggleControl? Cf Automattic/wp-calypso#28701

This comment has been minimized.

@jasmussen

jasmussen Nov 20, 2018

Contributor

Let me take a look as I address the other item from @simison, thanks!

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 20, 2018

Not sure I understand -- adding disabled styling was what I planned to do in the first place: #12105

Sorry I guess I just left this there without too much context :D

What i meant was that instead of wrapping the FormToggle in a Disabled component to disable it, perhaps having an actual disabled prop on the form toggle would be a better approach? Though I suppose both use cases need to work.

Address feedback.
- Add a better disabled state that works for both checked and unchecked states.
- Move code to the right component.
- Fix some additional bleed issues.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 20, 2018

Thanks for the review, everyone. Fixed a few issues, including those mentioned:

screenshot 2018-11-20 at 15 26 58

screenshot 2018-11-20 at 15 28 07

@ockham

ockham approved these changes Nov 20, 2018

Thanks @jasmussen, looks great! Any chance this makes it into 4.5? Would be great for people testing the Jetpack beta after today's code freeze so they won't run into Automattic/wp-calypso#28701 😄

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 20, 2018

Thanks for the review. I think that's up to @youknowriad.

@youknowriad

How can I go against three approvals?

@youknowriad youknowriad modified the milestones: 4.6, 4.5 Nov 20, 2018

@youknowriad youknowriad merged commit 3f74cd1 into master Nov 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/disabled-toggle-fix branch Nov 20, 2018

@ockham

This comment has been minimized.

Contributor

ockham commented Nov 20, 2018

Thanks @jasmussen and @youknowriad! ❤️

daniloercoli added a commit that referenced this pull request Nov 20, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/wire-on-replace-para-block

* 'master' of https://github.com/WordPress/gutenberg:
  Remove "permalink settings" link from permalink panel. (#12121)
  preserve quote content (#12122)
  Replace gutenberg domain with default for Core blocks (#12108)
  Fix issue with disabled togglecontrol double border (#12091)
  Fix the TinyMCE init array (#10968)

ockham added a commit to Automattic/wp-calypso that referenced this pull request Nov 20, 2018

Publicize Gutenblock: Use Disabled component rather than prop (#28723)
Use the `Disabled` component rather than a `disabled` prop to conditionally wrap `FormToggle` when the `toggleable` attribute is `false`. This should style the toggles properly when using Gutenberg `master` or a version that includes WordPress/gutenberg#12091.

@ockham ockham referenced this pull request Nov 20, 2018

Closed

Components: Add 'disabled' styling to FormToggle #12105

0 of 4 tasks complete

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Fix issue with disabled togglecontrol double border (WordPress#12091)
* Fix issue with disabled togglecontrol double border

Fixes WordPress#12049.

This PR fixes an issue where a togglecontrol, wrapped in a Disabled component, would show a double border. The issue was that it inherited a border from the parent checkbox style (because the toggle is technically a reskinned checkbox).

* Address feedback.

- Add a better disabled state that works for both checked and unchecked states.
- Move code to the right component.
- Fix some additional bleed issues.

jkmassel added a commit to Automattic/wp-calypso that referenced this pull request Nov 28, 2018

Publicize Gutenblock: Use Disabled component rather than prop (#28723)
Use the `Disabled` component rather than a `disabled` prop to conditionally wrap `FormToggle` when the `toggleable` attribute is `false`. This should style the toggles properly when using Gutenberg `master` or a version that includes WordPress/gutenberg#12091.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment