feat(checkbox, radio, toggle, range): stacked labels for form controls by thetaPC · Pull Request #28075 · ionic-team/ionic-framework

thetaPC · 2023-08-29T18:59:30Z

Issue number: resolves #27229

What is the current behavior?

Checkbox, radio, toggle, and range do not allow labels to be stacked.

What is the new behavior?

Checkbox can stack the label on top of the control and adjust the alignment.
- New prop align was added with center as the default¹
- New label-placement value was added
- Snapshots were added
Radio can stack the label on top of the control and adjust the alignment.
- New prop align was added with center as the default¹
- New label-placement value was added
- Snapshots were added
Toggle can stack the label on top of the control and adjust the alignment.
- New prop align was added with center as the default¹
- New label-placement value was added
- Snapshots were added
Range can stack the label on top of the control and adjust the alignment.
- New label-placement value was added
- Alignment cannot be changed. The label will be on the left side when LTR and right when RTL.
- Snapshots were added, padding was added to the container to prevent the knob from being cut off

Does this introduce a breaking change?

Yes
No

Other information

Original PR

Recommendation: Use the file filter during the PR review to filter out images. It'll make it easier to review code.
Associated PR to update ion-docs

center is being used because align-items="center" was the original style prior to the implementation. Otherwise, the label will not line up correctly on default. For example, the checkbox for iOS will shift the label to the top left corner when align defaulted to start.² This shift happens because the input is larger than the label, making the alignment obvious. But this is not correct, the label must be centered vertically as the default to prevent breaking changes to the current visualizations. ↩ ↩² ↩³
↩

bolt-new-by-stackblitz · 2023-08-29T19:32:01Z

Run & review this pull request in StackBlitz Codeflow.

liamdebeasi

The implementation of this is really good! We'll need to choose a different property name than align since it conflicts with a global deprecated attribute.

Feel free to add me for another review, but once my comments are addressed I think this is good to go.

liamdebeasi · 2023-08-30T19:44:29Z

core/src/components/toggle/toggle.tsx

+   * `"start"`: The label and control will appear at the top of the container.
+   * `"center"`: The label and control will appear at the center of the container.
+   */
+  @Prop() align: 'start' | 'center' = 'center';


We're going to need to name this something other than align because it collides with the global deprecated align attribute: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes. The align attribute is going to cause the text inside of the label container to be centered.

Maybe we can call this alignment? Open to other ideas from the team.

You'll also notice that long labels are no longer centered within their container. We're currently discussing the best path forward for this on Slack.

alignment works for me. And based on the Slack thread, no changes will be needed for centering.

liamdebeasi · 2023-08-30T19:50:52Z

core/src/components/toggle/toggle.tsx

  @Prop() justify: 'start' | 'end' | 'space-between' = 'space-between';

+  /**
+   * How to pack the label and control along the cross axis.


Can we update this comment (and ditto for other components) that this only applies when the label and control are on separate lines such as when using labelPlacement="stacked"?

liamdebeasi · 2023-08-30T19:51:19Z

core/src/components/toggle/toggle.tsx

   * `"start"`: The label will appear to the left of the toggle in LTR and to the right in RTL.
   * `"end"`: The label will appear to the right of the toggle in LTR and to the left in RTL.
   * `"fixed"`: The label has the same behavior as `"start"` except it also has a fixed width. Long text will be truncated with ellipses ("...").
+   * `"stacked"`: The label will appear above the toggle regardless of the direction. The alignment of the label can be controlled with the `align` property.


Don't forget to update this to reference "alignment" instead of "align" (or whatever name we decide on). Same thing for other components too

averyjohnston

LGTM once my comment is addressed 👍

averyjohnston · 2023-08-31T15:21:57Z

core/src/components/checkbox/checkbox.tsx


+  /**
+   * How to control the alignment of the checkbox and label on the cross axis.
+   * `"start"`: The label and control will appear at the top of the container.


"Top of the container" is a little misleading since it's only true for non-stacked label placements. Maybe something like "start of the cross axis"? Or "left side of the cross axis on LTR, or right side on RTL"? (Ditto for the other components.)

sean-perkins · 2023-08-31T16:55:24Z

core/src/components/checkbox/checkbox.scss

+  /**
+  * Label is on top of the checkbox.
+  */
+  :host(.checkbox-label-placement-stacked) .checkbox-wrapper {


White space looks strange here (for both the style and the comment block).

Thanks! Lint doesn't seem to catch it.

sean-perkins · 2023-08-31T17:01:46Z

core/src/components/range/range.scss

+:host(.range-label-placement-stacked) .range-wrapper {
+  flex-direction: column;
+
+  align-items: normal;


Any reason we used align-items: normal vs. align-items: stretch here?

Functionally they are the same, but normal is harder to descern as it changes the value based on the layout mode.

I went with normal because it's the default value for align-items. I'll switch it over to prevent any changes based on layout mode.

…nto FW-4090-alt

feat(checkbox): add support for stacked labels

d0581ab

github-actions bot added package: core @ionic/core package package: angular @ionic/angular package package: vue @ionic/vue package labels Aug 29, 2023

feat(radio): add support for stacked labels

f55abc7

thetaPC added 3 commits August 29, 2023 12:47

feat(range): add support for stacked labels

a5e73d7

feat(toggle): add support for stacked labels

77a3efa

feat(checkbox, radio): add missing prop option comment

6b47c7a

thetaPC mentioned this pull request Aug 29, 2023

feat(checkbox, radio, toggle, range): stacked labels for form controls #28021

Closed

2 tasks

feat(radio): remove only from test

58837f1

liamdebeasi force-pushed the FW-4090-alt branch 2 times, most recently from f60b9ea to 6a4dc5f Compare August 29, 2023 22:40

refactor: use toHaveScreenshot

25f0229

liamdebeasi force-pushed the FW-4090-alt branch from 1d5c847 to 25f0229 Compare August 29, 2023 23:21

Ionitron and others added 2 commits August 29, 2023 23:39

chore(): add updated snapshots

7ef9d1e

revert

8c23969

thetaPC marked this pull request as ready for review August 30, 2023 15:29

thetaPC requested review from averyjohnston, liamdebeasi and sean-perkins as code owners August 30, 2023 15:29

liamdebeasi approved these changes Aug 30, 2023

View reviewed changes

feat(checkbox, radio, toggle, range): requested changes

5a6e7f3

thetaPC requested a review from liamdebeasi August 30, 2023 22:23

averyjohnston approved these changes Aug 31, 2023

View reviewed changes

feat(checkbox, radio, toggle): update alignment comment

ebcfee5

sean-perkins reviewed Aug 31, 2023

View reviewed changes

thetaPC added 2 commits August 31, 2023 10:40

feat(checkbox): update indent

d07e5bf

feat(range): switch to stretch

f9ed625

liamdebeasi approved these changes Sep 1, 2023

View reviewed changes

Merge branch 'feature-7.4' of github.com:ionic-team/ionic-framework i…

29d0d72

…nto FW-4090-alt

thetaPC merged commit e6c7bb6 into feature-7.4 Sep 1, 2023

thetaPC deleted the FW-4090-alt branch September 1, 2023 16:31

This was referenced Sep 5, 2023

feat(checkbox, radio, toggle, range): stacked labels for form controls ionic-team/ionic-docs#3097

Merged

feat: stacked labels for form controls #27229

Closed

Conversation

thetaPC commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Footnotes

Uh oh!

bolt-new-by-stackblitz bot commented Aug 29, 2023

Uh oh!

liamdebeasi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamdebeasi Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

averyjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thetaPC commented Aug 29, 2023 •

edited

Loading

liamdebeasi Aug 30, 2023 •

edited

Loading