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

feat(checkbox): add helperText and errorText properties #30140

Open
wants to merge 25 commits into
base: feature-8.5
Choose a base branch
from

Conversation

brandyscarney
Copy link
Member

@brandyscarney brandyscarney commented Jan 20, 2025

Issue number: resolves #29810


What is the current behavior?

Checkbox does not support helper and error text.

What is the new behavior?

Adds support for helper and error text, similar to input and textarea.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Copy link

vercel bot commented Jan 20, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 29, 2025 7:38pm

@github-actions github-actions bot added package: core @ionic/core package package: angular @ionic/angular package package: vue @ionic/vue package labels Jan 20, 2025
}

:host(.checkbox-label-placement-stacked) .checkbox-bottom {
font-size: dynamic-font(16px);
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this because without it the text is really small.

@@ -260,6 +309,8 @@ input {
*/
:host(.checkbox-label-placement-stacked) .checkbox-wrapper {
flex-direction: column;

text-align: center;
Copy link
Member Author

@brandyscarney brandyscarney Jan 28, 2025

Choose a reason for hiding this comment

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

This text alignment change is necessary because without it the label is left aligned and when you change between helper/error text it jumps:

helper-stacked.mov

After my changes:

helper-stacked-fixed.mov

This issue does not exist without helper/error text.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense to me. Everything is centered already so the supporting text should follow that.

}

return (
<div class="checkbox-bottom">
Copy link
Member Author

Choose a reason for hiding this comment

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

The class checkbox-bottom doesn't make as much sense here since it's not on the bottom of the entire checkbox but it is under the label still and I named it this to remain consistent with input, textarea, etc.

@@ -217,6 +224,8 @@ input {
*/
:host(.checkbox-label-placement-end) .checkbox-wrapper {
flex-direction: row-reverse;

justify-content: start;
Copy link
Member Author

@brandyscarney brandyscarney Jan 28, 2025

Choose a reason for hiding this comment

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

This might cause unwanted changes in some use cases, but this matches what is expected in the Figma design. Here is how it looks before and after this change with width: 100% on the checkbox:

Before After
helper-text-label-end-before helper-text-label-end-after
label-placement-end-before label-placement-end-after

Because this changes the default behavior in an item, maybe we want to change this later and just recommend they set the justify property when using it with helper text? I am not sure, but it jumps around with the space-between default:

helper.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

@brandyscarney which Figma design are you referring to?

Copy link
Member Author

Choose a reason for hiding this comment

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

@thetaPC I will send you a link

Copy link
Contributor

Choose a reason for hiding this comment

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

After some review, I agree with these changes. It also seems like a fix so it's a win win.

@@ -287,6 +298,46 @@ input {
@include transform-origin(center, top);
}

// Justify Content
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't make changes to any of this code I just moved it lower so it would take priority over the justify-content I specified in label-placement-end.

@brandyscarney brandyscarney marked this pull request as ready for review January 28, 2025 21:50
@brandyscarney brandyscarney requested a review from a team as a code owner January 28, 2025 21:50
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

The supporting text being read twice is the main issue. The rest are just questions.

@@ -228,6 +287,7 @@ export class Checkbox implements ComponentInterface {
part="label"
>
<slot></slot>
{this.renderHintText()}
Copy link
Contributor

Choose a reason for hiding this comment

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

The text (helper or error) is being read twice by VoiceOver on Safari, Firefox, and Chrome. After some quick debugging, it seems that this happening because the text is inside of label. ion-input has it after the <label> which mimics how the native input states it.

Screen.Recording.2025-01-30.at.12.26.40.PM.mov

@@ -228,6 +287,7 @@ export class Checkbox implements ComponentInterface {
part="label"
>
<slot></slot>
{this.renderHintText()}
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the focus with VoiceOver does not highlight correctly. This is happening on the main branch. We should consider creating a spike to fix it.

configs({ modes: ['ios', 'md'], directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
test.describe(title('checkbox: helper text rendering'), () => {
// Check the default label placement, end, and stacked
[undefined, 'end', 'stacked'].forEach((labelPlacement) => {
Copy link
Contributor

@thetaPC thetaPC Jan 30, 2025

Choose a reason for hiding this comment

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

What was the reasoning for not checking fixed?

});
});

test.describe(title('checkbox: error text rendering'), () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does error text not get a check for the end placement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants