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

Update help text alignment in CheckboxControl #60787

Merged
merged 12 commits into from
Apr 22, 2024
Merged

Update help text alignment in CheckboxControl #60787

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
599a832
Update help text alignment
jameskoster Apr 16, 2024
456b9de
Improve alignment, fix wrapping
jameskoster Apr 17, 2024
95f19f9
Changelog
jameskoster Apr 17, 2024
db42921
Update packages/components/CHANGELOG.md
jameskoster Apr 17, 2024
55e5dc7
Wrap checkbox + label in HStack, adjust css
jameskoster Apr 18, 2024
8a7f550
Internalize HStack import
mirka Apr 18, 2024
8f08d50
Use styled span for help text
mirka Apr 18, 2024
92d2783
Tweak changelog
mirka Apr 18, 2024
6c41d46
Merge branch 'trunk' into update/checkbox-help-text-alignment
mirka Apr 19, 2024
be6becb
Don't add span when no help text
mirka Apr 19, 2024
e3a1640
Add unit test
mirka Apr 19, 2024
b20d045
Update snapshots
mirka Apr 19, 2024
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
- `Navigator`: Navigation to the active path doesn't create a new location history. ([#60561](https://github.com/WordPress/gutenberg/pull/60561))
- `FormToggle`: Forwards ref to input. ([#60234](https://github.com/WordPress/gutenberg/pull/60234)).
- `ToggleControl`: Forwards ref to FormToggle. ([#60234](https://github.com/WordPress/gutenberg/pull/60234)).
- `CheckboxControl`: Update help text alignment. ([#60786](https://github.com/WordPress/gutenberg/pull/60787)).
jameskoster marked this conversation as resolved.
Show resolved Hide resolved

### Bug Fix

Expand Down
17 changes: 16 additions & 1 deletion packages/components/src/checkbox-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,20 @@
@include break-small() {
--checkbox-input-size: 20px;
}

.components-base-control__help {
margin-left: calc(var(--checkbox-input-size) + #{$grid-unit-15});
}
Copy link
Member

Choose a reason for hiding this comment

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

This is inevitable at the moment, but raised as an issue at #60836.

Copy link
Contributor

Choose a reason for hiding this comment

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

Saw the linked issue. It probably wouldn't be as flexible, but could checkbox control do something like this to indent the text?

help={ <span="components-checkbox-control__help-text">{ helpText }</span> }
.components-checkbox-control__help {
    margin-left: calc(var(--checkbox-input-size) + #{$grid-unit-15});
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh, good one! Yes that could work in this instance, with a display: inline-block to account for when the help text is long enough to wrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand the benefit to doing this? Feel free to push any changes :)

Copy link
Member

Choose a reason for hiding this comment

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

Good question. The point is that we shouldn't be using selectors from other components, because they aren't guaranteed to be stable. Ideally they would all be obfuscated/randomized so they aren't even available outside the original component.

I'll try it out and push the changes 👍

Copy link
Member

Choose a reason for hiding this comment

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

Done in 8f08d50


.components-base-control__field {
// Emsure label doesn't wrap beneath checkbox
display: flex;
}
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid reaching into BaseControl internals here by wrapping our elements in an <HStack spacing={ 0 } justify="start" alignment="top"> instead.


.components-checkbox-control__label {
// Ensure label is aligned with checkbox along X axis
line-height: var(--checkbox-input-size);
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this part out of the .components-checkbox-control block to avoid unnecessary specificity.

}

.components-checkbox-control__input[type="checkbox"] {
Expand Down Expand Up @@ -50,9 +64,10 @@
.components-checkbox-control__input-container {
position: relative;
display: inline-block;
margin-right: 12px;
margin-right: $grid-unit-15;
vertical-align: middle;
width: var(--checkbox-input-size);
height: var(--checkbox-input-size);
aspect-ratio: 1;
Copy link
Member

Choose a reason for hiding this comment

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

Was this height necessary? It makes the aspect-ratio redundant.

I think we also need a flex-shrink: 0 here to prevent squishing:

Element being squished

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was, but moving to a HStack seems to have made it redundant :)

}

Expand Down
Loading