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

CheckboxControl: replace margin overrides with new opt-in prop #45434

Merged
merged 2 commits into from
Nov 3, 2022
Merged

CheckboxControl: replace margin overrides with new opt-in prop #45434

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cecb6c
CheckboxControl: replace margin overrides with new opt-in prop
brookewp Nov 1, 2022
99ac2c3
Fix failing snapshots
brookewp Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/block-lock/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export default function BlockLockModal( { clientId, onClose } ) {
className="block-editor-block-lock-modal__options"
>
<CheckboxControl
__nextHasNoMarginBottom
className="block-editor-block-lock-modal__options-title"
label={
<span id={ instanceId }>{ __( 'Lock all' ) }</span>
Expand All @@ -134,6 +135,7 @@ export default function BlockLockModal( { clientId, onClose } ) {
{ allowsEditLocking && (
<li className="block-editor-block-lock-modal__checklist-item">
<CheckboxControl
__nextHasNoMarginBottom
label={
<>
{ __( 'Restrict editing' ) }
Expand All @@ -158,6 +160,7 @@ export default function BlockLockModal( { clientId, onClose } ) {
) }
<li className="block-editor-block-lock-modal__checklist-item">
<CheckboxControl
__nextHasNoMarginBottom
label={
<>
{ __( 'Disable movement' ) }
Expand All @@ -181,6 +184,7 @@ export default function BlockLockModal( { clientId, onClose } ) {
</li>
<li className="block-editor-block-lock-modal__checklist-item">
<CheckboxControl
__nextHasNoMarginBottom
label={
<>
{ __( 'Prevent removal' ) }
Expand Down
2 changes: 0 additions & 2 deletions packages/block-editor/src/components/block-lock/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
.components-base-control__field {
align-items: center;
display: flex;
Comment on lines 22 to 24
Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but a good follow-up for these would be to move the Icon out of the CheckboxControl label (it doesn't make sense), and move these flex styles into the parent element so we can remove these hacky overrides on components-base-control__field.

(Same with block-manager)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mirka when I initially read your comment, I assumed it was related to accessibility. I've just tested with my screen reader and looked a bit into it, but I couldn't find anything. Would you mind sharing why it is best to move the icon out of the label (aside from the opportunity to remove the CSS overrides)?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, it's not an a11y issue, just that there is no reason for the aria-hidden icon to be in the label. The main motivation is to remove the CSS override. I'll add some context in case some of it was missing:

Any consumer usage that reaches into and overrides the internal styling of a wp-component is inherently brittle. If BaseControl changes its structure or styling at some point, these BlockLock overrides could break in unexpected ways. We've been discouraging style hacks like this, since it actively harms our ability to make changes to wp-components without things breaking all over the place. It's doubly harmful because third-party devs often turn to the Gutenberg source code to learn how they should be building things.

So when things look like you might want to override internal wp-component styling, the decision flow would roughly be like:

  1. Can I accomplish the same thing without adding a style hack? If so, that is always better.
  2. Is this a common design pattern that will appear multiple times in the Gutenberg app? If so, consider proposing a change to the wp-component in question so it can flexibly handle more use cases out of the box.
  3. If no to either of those questions, consider building your own component from scratch, or modifying your design so it works within the existing component limitations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the detailed explanation! Very helpful! 🙌

margin: 0;
}
}
.block-editor-block-lock-modal__checklist-item {
Expand All @@ -32,7 +31,6 @@
.components-base-control__field {
align-items: center;
display: flex;
margin: 0;
}

.components-checkbox-control__label {
Expand Down
1 change: 1 addition & 0 deletions packages/edit-post/src/components/block-manager/category.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ function BlockManagerCategory( { title, blockTypes } ) {
className="edit-post-block-manager__category"
>
<CheckboxControl
__nextHasNoMarginBottom
checked={ isAllChecked }
onChange={ toggleAllVisible }
className="edit-post-block-manager__category-title"
Expand Down
1 change: 1 addition & 0 deletions packages/edit-post/src/components/block-manager/checklist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ function BlockTypesChecklist( { blockTypes, value, onItemChange } ) {
className="edit-post-block-manager__checklist-item"
>
<CheckboxControl
__nextHasNoMarginBottom
label={
<>
{ blockType.title }
Expand Down
1 change: 0 additions & 1 deletion packages/edit-post/src/components/block-manager/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
.components-base-control__field {
align-items: center;
display: flex;
margin: 0;
}
}

Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/entities-saved-states/entity-record-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ export default function EntityRecordItem( {
return (
<PanelRow>
<CheckboxControl
__nextHasNoMarginBottom
label={
<strong>
{ decodeEntities( entityRecordTitle ) ||
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-comments/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function PostComments( { commentStatus = 'open', ...props } ) {

return (
<CheckboxControl
__nextHasNoMarginBottom
label={ __( 'Allow comments' ) }
checked={ commentStatus === 'open' }
onChange={ onToggleComments }
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-pending-status/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export function PostPendingStatus( { status, onUpdateStatus } ) {
return (
<PostPendingStatusCheck>
<CheckboxControl
__nextHasNoMarginBottom
label={ __( 'Pending review' ) }
checked={ status === 'pending' }
onChange={ togglePendingStatus }
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-pingbacks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ function PostPingbacks( { pingStatus = 'open', ...props } ) {

return (
<CheckboxControl
__nextHasNoMarginBottom
label={ __( 'Allow pingbacks & trackbacks' ) }
checked={ pingStatus === 'open' }
onChange={ onTogglePingback }
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-publish-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ export class PostPublishPanel extends Component {
</div>
<div className="editor-post-publish-panel__footer">
<CheckboxControl
__nextHasNoMarginBottom
label={ __( 'Always show pre-publish checks.' ) }
checked={ isPublishSidebarEnabled }
onChange={ onTogglePublishSidebar }
Expand Down
24 changes: 10 additions & 14 deletions packages/editor/src/components/post-publish-panel/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ exports[`PostPublishPanel should render the post-publish panel if the post is pu
padding: 0;
}

.components-panel__row .emotion-8 {
margin-bottom: inherit;
}

<div>
<div
class="editor-post-publish-panel"
Expand Down Expand Up @@ -141,7 +145,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is pu
class="components-base-control components-checkbox-control emotion-0 emotion-1"
>
<div
class="components-base-control__field emotion-2 emotion-3"
class="components-base-control__field emotion-8 emotion-3"
>
<span
class="components-checkbox-control__input-container"
Expand Down Expand Up @@ -197,6 +201,10 @@ exports[`PostPublishPanel should render the post-publish panel if the post is sc
padding: 0;
}

.components-panel__row .emotion-8 {
margin-bottom: inherit;
}

<div>
<div
class="editor-post-publish-panel"
Expand Down Expand Up @@ -307,7 +315,7 @@ exports[`PostPublishPanel should render the post-publish panel if the post is sc
class="components-base-control components-checkbox-control emotion-0 emotion-1"
>
<div
class="components-base-control__field emotion-2 emotion-3"
class="components-base-control__field emotion-8 emotion-3"
>
<span
class="components-checkbox-control__input-container"
Expand Down Expand Up @@ -345,10 +353,6 @@ exports[`PostPublishPanel should render the pre-publish panel if post status is
box-sizing: inherit;
}

.emotion-2 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-2 {
margin-bottom: inherit;
}
Expand Down Expand Up @@ -472,10 +476,6 @@ exports[`PostPublishPanel should render the pre-publish panel if the post is not
box-sizing: inherit;
}

.emotion-2 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-2 {
margin-bottom: inherit;
}
Expand Down Expand Up @@ -641,10 +641,6 @@ exports[`PostPublishPanel should render the spinner if the post is being saved 1
box-sizing: inherit;
}

.emotion-8 {
margin-bottom: calc(4px * 2);
}

.components-panel__row .emotion-8 {
margin-bottom: inherit;
}
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-sticky/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export function PostSticky( { onUpdateSticky, postSticky = false } ) {
return (
<PostStickyCheck>
<CheckboxControl
__nextHasNoMarginBottom
label={ __( 'Stick to the top of the blog' ) }
checked={ postSticky }
onChange={ () => onUpdateSticky( ! postSticky ) }
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/components/post-taxonomies/hierarchical-term-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ export function HierarchicalTermSelector( { slug } ) {
className="editor-post-taxonomies__hierarchical-terms-choice"
>
<CheckboxControl
__nextHasNoMarginBottom
checked={ terms.indexOf( term.id ) !== -1 }
onChange={ () => {
const termId = parseInt( term.id, 10 );
Expand Down