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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
2aaf9e4
feat(checkbox): add helperText and errorText
brandyscarney Jan 20, 2025
315642c
test(checkbox): add bottom content tests
brandyscarney Jan 20, 2025
7e86b42
chore(): add updated snapshots
brandyscarney Jan 20, 2025
b3aeac1
style: lint
brandyscarney Jan 21, 2025
e4531ea
Merge branch 'feature-8.5' into ROU-11141
brandyscarney Jan 21, 2025
804ebbd
test(checkbox): delete copied test
brandyscarney Jan 21, 2025
a09d2da
refactor(checkbox): remove added css vars
brandyscarney Jan 22, 2025
b67d888
style: revert
brandyscarney Jan 22, 2025
e72fe10
chore: build
brandyscarney Jan 22, 2025
bfc9617
chore(): remove snapshots
brandyscarney Jan 27, 2025
0f03052
test(checkbox): only toggle between helper and error with both
brandyscarney Jan 27, 2025
4794ed7
Merge branch 'feature-8.5' into ROU-11141
brandyscarney Jan 27, 2025
444e213
fix(checkbox): align text based on label placement
brandyscarney Jan 27, 2025
54da960
feat(checkbox): add shadow parts
brandyscarney Jan 27, 2025
662d1ff
test(checkbox): update e2e test
brandyscarney Jan 27, 2025
bfc8cf7
chore(): add updated snapshots
brandyscarney Jan 27, 2025
d61c38d
style: lint
brandyscarney Jan 28, 2025
45141d5
style: remove comment
brandyscarney Jan 28, 2025
e08d608
fix(checkbox): change justify instead of text alignment
brandyscarney Jan 28, 2025
4ece469
test(checkbox): add more examples to item test
brandyscarney Jan 28, 2025
e32256f
chore(): add updated snapshots
brandyscarney Jan 28, 2025
39e6d9f
test(checkbox): add e2e test and screenshots for end placement
brandyscarney Jan 28, 2025
7996747
docs: helper and error text description
brandyscarney Jan 29, 2025
5f86dd9
fix(checkbox): reduce padding to match md guidelines
brandyscarney Jan 29, 2025
d771da6
Merge branch 'feature-8.5' into ROU-11141
brandyscarney Jan 29, 2025
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
5 changes: 5 additions & 0 deletions core/api.txt
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,8 @@ ion-checkbox,prop,alignment,"center" | "start" | undefined,undefined,false,false
ion-checkbox,prop,checked,boolean,false,false,false
ion-checkbox,prop,color,"danger" | "dark" | "light" | "medium" | "primary" | "secondary" | "success" | "tertiary" | "warning" | string & Record<never, never> | undefined,undefined,false,true
ion-checkbox,prop,disabled,boolean,false,false,false
ion-checkbox,prop,errorText,string | undefined,undefined,false,false
ion-checkbox,prop,helperText,string | undefined,undefined,false,false
ion-checkbox,prop,indeterminate,boolean,false,false,false
ion-checkbox,prop,justify,"end" | "space-between" | "start" | undefined,undefined,false,false
ion-checkbox,prop,labelPlacement,"end" | "fixed" | "stacked" | "start",'start',false,false
Expand Down Expand Up @@ -430,8 +432,11 @@ ion-checkbox,css-prop,--size,md
ion-checkbox,css-prop,--transition,ios
ion-checkbox,css-prop,--transition,md
ion-checkbox,part,container
ion-checkbox,part,error-text
ion-checkbox,part,helper-text
ion-checkbox,part,label
ion-checkbox,part,mark
ion-checkbox,part,supporting-text

ion-chip,shadow
ion-chip,prop,color,"danger" | "dark" | "light" | "medium" | "primary" | "secondary" | "success" | "tertiary" | "warning" | string & Record<never, never> | undefined,undefined,false,true
Expand Down
16 changes: 16 additions & 0 deletions core/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,14 @@ export namespace Components {
* If `true`, the user cannot interact with the checkbox.
*/
"disabled": boolean;
/**
* Text that is placed under the checkbox label and displayed when an error is detected.
*/
"errorText"?: string;
/**
* Text that is placed under the checkbox label and displayed when no error is detected.
*/
"helperText"?: string;
/**
* If `true`, the checkbox will visually appear as indeterminate.
*/
Expand Down Expand Up @@ -5403,6 +5411,14 @@ declare namespace LocalJSX {
* If `true`, the user cannot interact with the checkbox.
*/
"disabled"?: boolean;
/**
* Text that is placed under the checkbox label and displayed when an error is detected.
*/
"errorText"?: string;
/**
* Text that is placed under the checkbox label and displayed when no error is detected.
*/
"helperText"?: string;
/**
* If `true`, the checkbox will visually appear as indeterminate.
*/
Expand Down
101 changes: 76 additions & 25 deletions core/src/components/checkbox/checkbox.scss
Original file line number Diff line number Diff line change
Expand Up @@ -148,46 +148,53 @@ input {
opacity: 0;
}

// Justify Content
// ---------------------------------------------
// Checkbox Bottom Content
// ----------------------------------------------------------------

.checkbox-bottom {
@include padding(4px, null, null, null);

display: flex;

:host(.checkbox-justify-space-between) .checkbox-wrapper {
justify-content: space-between;
}

:host(.checkbox-justify-start) .checkbox-wrapper {
justify-content: start;
font-size: dynamic-font(12px);

white-space: normal;
}

:host(.checkbox-justify-end) .checkbox-wrapper {
justify-content: end;
: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.

}

// Align Items
// ---------------------------------------------
// Checkbox Hint Text
// ----------------------------------------------------------------

:host(.checkbox-alignment-start) .checkbox-wrapper {
align-items: start;
}
/**
* Error text should only be shown when .ion-invalid is
* present on the checkbox. Otherwise the helper text should
* be shown.
*/
.checkbox-bottom .error-text {
display: none;

:host(.checkbox-alignment-center) .checkbox-wrapper {
align-items: center;
color: ion-color(danger, base);
}

// Justify Content & Align Items
// ---------------------------------------------
.checkbox-bottom .helper-text {
display: block;

// The checkbox should be displayed as block when either justify
// or alignment is set; otherwise, these properties will have no
// visible effect.
:host(.checkbox-justify-space-between),
:host(.checkbox-justify-start),
:host(.checkbox-justify-end),
:host(.checkbox-alignment-start),
:host(.checkbox-alignment-center) {
color: $text-color-step-300;
}

:host(.ion-touched.ion-invalid) .checkbox-bottom .error-text {
display: block;
}

:host(.ion-touched.ion-invalid) .checkbox-bottom .helper-text {
display: none;
}

// Label Placement - Start
// ----------------------------------------------------------------

Expand Down Expand Up @@ -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.

}

/**
Expand Down Expand Up @@ -260,6 +269,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.

}

:host(.checkbox-label-placement-stacked) .label-text-wrapper {
Expand Down Expand Up @@ -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.

// ---------------------------------------------

:host(.checkbox-justify-space-between) .checkbox-wrapper {
justify-content: space-between;
}

:host(.checkbox-justify-start) .checkbox-wrapper {
justify-content: start;
}

:host(.checkbox-justify-end) .checkbox-wrapper {
justify-content: end;
}

// Align Items
// ---------------------------------------------

:host(.checkbox-alignment-start) .checkbox-wrapper {
align-items: start;
}

:host(.checkbox-alignment-center) .checkbox-wrapper {
align-items: center;
}

// Justify Content & Align Items
// ---------------------------------------------

// The checkbox should be displayed as block when either justify
// or alignment is set; otherwise, these properties will have no
// visible effect.
:host(.checkbox-justify-space-between),
:host(.checkbox-justify-start),
:host(.checkbox-justify-end),
:host(.checkbox-alignment-start),
:host(.checkbox-alignment-center) {
display: block;
}

// Checked / Indeterminate Checkbox
// ---------------------------------------------

Expand Down
60 changes: 60 additions & 0 deletions core/src/components/checkbox/checkbox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ import type { CheckboxChangeEventDetail } from './checkbox-interface';
* @part container - The container for the checkbox mark.
* @part label - The label text describing the checkbox.
* @part mark - The checkmark used to indicate the checked state.
* @part supporting-text - Supporting text displayed beneath the checkbox label.
* @part helper-text - Supporting text displayed beneath the checkbox label when the checkbox is valid.
* @part error-text - Supporting text displayed beneath the checkbox label when the checkbox is invalid and touched.
*/
@Component({
tag: 'ion-checkbox',
Expand All @@ -28,6 +31,8 @@ import type { CheckboxChangeEventDetail } from './checkbox-interface';
})
export class Checkbox implements ComponentInterface {
private inputId = `ion-cb-${checkboxIds++}`;
private helperTextId = `${this.inputId}-helper-text`;
private errorTextId = `${this.inputId}-error-text`;
private focusEl?: HTMLElement;
private inheritedAttributes: Attributes = {};

Expand Down Expand Up @@ -60,6 +65,16 @@ export class Checkbox implements ComponentInterface {
*/
@Prop() disabled = false;

/**
* Text that is placed under the checkbox label and displayed when an error is detected.
*/
@Prop() errorText?: string;

/**
* Text that is placed under the checkbox label and displayed when no error is detected.
*/
@Prop() helperText?: string;

/**
* The value of the checkbox does not mean if it's checked or not, use the `checked`
* property for that.
Expand Down Expand Up @@ -167,6 +182,48 @@ export class Checkbox implements ComponentInterface {
this.toggleChecked(ev);
};

private getHintTextID(): string | undefined {
const { el, helperText, errorText, helperTextId, errorTextId } = this;

if (el.classList.contains('ion-touched') && el.classList.contains('ion-invalid') && errorText) {
return errorTextId;
}

if (helperText) {
return helperTextId;
}

return undefined;
}

/**
* Responsible for rendering helper text and error text.
* This element should only be rendered if hint text is set.
*/
private renderHintText() {
const { helperText, errorText, helperTextId, errorTextId } = this;

/**
* undefined and empty string values should
* be treated as not having helper/error text.
*/
const hasHintText = !!helperText || !!errorText;
if (!hasHintText) {
return;
}

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.

<div id={helperTextId} class="helper-text" part="supporting-text helper-text">
{helperText}
</div>
<div id={errorTextId} class="error-text" part="supporting-text error-text">
{errorText}
</div>
</div>
);
}

render() {
const {
color,
Expand Down Expand Up @@ -218,6 +275,8 @@ export class Checkbox implements ComponentInterface {
onFocus={() => this.onFocus()}
onBlur={() => this.onBlur()}
ref={(focusEl) => (this.focusEl = focusEl)}
aria-describedby={this.getHintTextID()}
aria-invalid={this.getHintTextID() === this.errorTextId}
{...inheritedAttributes}
/>
<div
Expand All @@ -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

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.

</div>
<div class="native-wrapper">
<svg class="checkbox-icon" viewBox="0 0 24 24" part="container">
Expand Down
Loading
Loading