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

Fix checkbox alignment issues in options and block manager modals (#16860) #16863

Merged
merged 9 commits into from Aug 9, 2019

Conversation

@jffng
Copy link
Contributor

commented Aug 1, 2019

Description

Checkboxes are misaligned in the options + block manager modals due to an attempt to replace them with SVG (#16551).

This PR introduces editor specific css to address those alignment issues.

How has this been tested?

Tested MacOS on latest Chrome, Firefox, and Safari.

Screenshots

Screen Shot 2019-08-01 at 12 23 25 PM

Screen Shot 2019-08-01 at 12 23 15 PM

Types of changes

Fixes #16860

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

cc @kjellr

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Thanks for the quick fix, @jffng. Just a small nitpicky note:

I'm seeing a little bit of spacing inconsistency, and I can't figure out where it's coming from. Check out these two adjacent list items in the Options panel for example:

Screen Shot 2019-08-01 at 1 58 18 PM

Notice that the first checkmark is appearing higher than the one below it. Those should theoretically have the exact same styles, so I'm not sure what's causing the inconsistency. Any ideas?

@jffng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@kjellr it looks like the actual inputs are being rendered in ever so slightly different positions, thus throwing off the position of the checkmark relative to the box:

Screen Shot 2019-08-02 at 10 55 15 AM

The issue goes away when the width + height of the input is greater than some threshold, e.g. 19 x 19px:

Screen Shot 2019-08-02 at 10 58 41 AM

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

it looks like the actual inputs are being rendered in ever so slightly different positions, thus throwing off the position of the checkmark relative to the box:

That is truly bizarre. 🤔 In any case, I don't think it should hold up this PR. I posted a tiny comment about the z-index above. Once that's taken care of this should be good to ship!

Set height of label
Use z-index mixin
@jffng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@kjellr I think I came up with a fix. The height of the labels was fractionally different in some places due to a rem padding calculation:

Screen Shot 2019-08-02 at 4 06 14 PM

Manually setting the height of the label seems to have done the trick.

Mind giving it one more spin to see if passes the eye test?

Screen Shot 2019-08-02 at 4 17 36 PM

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

It's a bit concerning that the height of the label affects the checkbox, I was expecting that the checkbox component would work properly no matter the context it's being used in? Does this mean any developer using the component would have to adjust the height of the labels somehow?

@jffng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

@youknowriad this is a valid concern. I pushed an update that should address it, let me know if it's okay. Previously I wanted to avoid adding any additional markup to the component, but looking at other checkbox implementations, I think it's reasonable to add a span that wraps the input and svg elements in a single inline container.

@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

I'll let Riad weigh in on the method, but I can confirm that this fixes the spacing issue. Visually, this looks good!

Screen Shot 2019-08-05 at 11 39 23 AM

Screen Shot 2019-08-05 at 11 39 10 AM

@@ -1,3 +1,6 @@
$input-size: 16px;

This comment has been minimized.

Copy link
@kjellr

kjellr Aug 5, 2019

Contributor

Should these be declared somewhere more global (ie. _variables.scss)?

@@ -54,6 +54,7 @@
top: 0;
padding: $panel-padding 0;
background-color: $white;
z-index: z-index(".edit-post-manage-blocks-modal__category-title");

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 5, 2019

Contributor

How is this change related?

This comment has been minimized.

Copy link
@jffng

jffng Aug 5, 2019

Author Contributor

The headings for the block manager modal use position: sticky, creating a "sticky container" parent. Using a position: relative element (the wrapper for the input to position the checkbox) within the same sticky parent container, the browser treats them as in the same stacking context:

Screen Recording 2019-08-05 at 03 17 PM

Adding a z-index to the header makes the sticky header remain on top:

Screen Recording 2019-08-05 at 03 20 PM

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

The approach seems good to me. I'm wondering why do we need changes outside the component itself?

@youknowriad youknowriad closed this Aug 5, 2019

@youknowriad youknowriad reopened this Aug 5, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

(Sorry, clicked the wrong button)

@@ -65,3 +65,5 @@ $block-bg-padding--h: $block-side-ui-width + $block-side-ui-clearance; // paddin
// Buttons & UI Widgets
$radius-round-rectangle: 4px;
$radius-round: 50%;
$input-size: 16px;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 6, 2019

Contributor

To be honest, I'm not certain about these variables being global here. I think it only make sense if we use them consistently in all "inputs" otherwise it's going to create confusion. For example, why the size of the TextControl component doesn't use it?...

Thoughts @kjellr maybe we can leave them in the component for now, and maybe rename them to make it clear that it's just about the checkbox?

This comment has been minimized.

Copy link
@kjellr

kjellr Aug 6, 2019

Contributor

Yeah, I just conferred with @jasmussen about this, and while we haven't usually declared variables within components in the past, we can see how it would make sense to allow them.

Feel free to move these back, @jffng (and sorry for the back and forth!). Might also want to make the names just a little more specific too, as Riad noted.

Thanks!

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Looks like there are some failing unit tests preventing us from merging this branch. Is this something you can take care of or need help with?

@jffng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

@youknowriad
Copy link
Contributor

left a comment

Thanks for the updates. LGTM 👍

@youknowriad youknowriad merged commit c1d4497 into WordPress:master Aug 9, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@kjellr

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

🎉 Thanks for sorting this out, @jffng!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.