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

CheckboxControl: move icons out of labels #45535

Merged
merged 5 commits into from Nov 9, 2022

Conversation

brookewp
Copy link
Contributor

@brookewp brookewp commented Nov 4, 2022

What?

The icons for BlockLockModal and BlockTypesChecklist were nested inside the CheckboxControl label. This PR moves them out of the label and removes unnecessary CSS.

Why?

This PR is based on the comment here: #45434 (comment) by @mirka

How?

Moving the Icon and BlockIcon components out of the label prop. Also, removing the CSS for components-base-control__field and components-checkbox-control__label and moving it to the parent class for the respective components.

Testing Instructions

For BlockLockModal

  1. Add the Navigation block to the editor (this block has the extra setting Restrict editing that some other blocks don't have, i.e the Paragraph block)
  2. Click on the three dots and then on 'Lock'
  3. Check the different checkboxes to test the options:
    -- Make sure the icon shows the correct lock based on if it's locked or unlocked
    -- Ensure background hover works as expected
    -- Changes are responsive

Screen Shot 2022-11-03 at 5 54 56 PM

For BlockTypesChecklist

  1. Click on three dots in the top right corner of post editor
  2. Then select 'Preferences'
  3. Click on 'Blocks' and scroll down to 'Visible Blocks'
  4. Check for the following:
    -- Ensure it looks the same as before (it's about a pixel different in vertical spacing, not sure if that'll be an issue)
    -- Check that it's responsive

Screen Shot 2022-11-03 at 6 22 44 PM

@codesandbox
Copy link

codesandbox bot commented Nov 4, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @brookewp! I like how clean things look now.

A minor thing: It feels that spacing is a bit off on this branch compared to the trunk.

trunk this branch
CleanShot 2022-11-04 at 11 20 25 CleanShot 2022-11-04 at 11 21 08

@mirka mirka added [Package] Block editor /packages/block-editor [Package] Edit Post /packages/edit-post [Type] Code Quality Issues or PRs that relate to code quality labels Nov 4, 2022
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Thanks for the nice cleanup!

+1 on the vertical alignment thing from @Mamaduka, though the screenshots seem flipped 😆

margin-right: $grid-unit-15;
fill: $gray-900;
}
svg {
Copy link
Member

Choose a reason for hiding this comment

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

We need to be careful with element selectors like svg, as in this case where it's unintentionally applying to the checkmark svg as well 😄

The normal suggested way would be to add our own className to Icon for better scoping.

It might also be nice to add a flex-grow: 0 in here so the icon doesn't shrink when the checkbox text is long.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to be careful with element selectors like svg, as in this case where it's unintentionally applying to the checkmark svg as well 😄

Good catch! I usually have a good eye for that, so I'm surprised I missed that! 🤦‍♀️

It might also be nice to add a flex-grow: 0 in here so the icon doesn't shrink when the checkbox text is long.

I might be missing something, but I'm having trouble getting flex-grow: 0 to work in that scenario. Would it be a bad idea to set a min-width on the icon instead?

Copy link
Member

Choose a reason for hiding this comment

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

I think I meant flex-shrink 🙈 Does that work?

}
svg {
margin-right: $grid-unit-15;
fill: $gray-900;
Copy link
Member

Choose a reason for hiding this comment

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

The fill hack is not ideal (#40102), but I understand why it's needed at the moment. (No action required, just mentioning for future context.)

@@ -25,24 +25,14 @@
}
}
.block-editor-block-lock-modal__checklist-item {
display: flex;
justify-content: space-between;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth adding a flex-gap so the text and icon don't run together when the text is long?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me!

@brookewp
Copy link
Contributor Author

brookewp commented Nov 5, 2022

Thank you both @Mamaduka and @mirka for your suggestions! When looking at the text it didn't seem too different but when seeing it highlighted it's very clearly misaligned. I've center-aligned it now but I think it is a pixel off. Will that be an issue? What do you think?

Screen Shot 2022-11-04 at 7 45 35 PM

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thank you, @brookewp!

I've center-aligned it now but I think it is a pixel off. Will that be an issue? What do you think?

That's okay with me. The items are correctly aligned.

@brookewp brookewp force-pushed the checkboxcontrol/move-label-icons branch from 9285615 to bfef1c6 Compare November 9, 2022 18:53
@brookewp brookewp requested a review from mirka November 9, 2022 18:54
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! 🚀

@mirka mirka merged commit bdd3fd7 into WordPress:trunk Nov 9, 2022
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Package] Edit Post /packages/edit-post [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants