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

Fix: Apply text-disabled to labels and hint blocks when components are disabled #194

Merged
merged 1 commit into from
Jun 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/pretty-cheetahs-burn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@crowdstrike/ember-toucan-core': patch
'@crowdstrike/ember-toucan-form': patch
---

Updates disabled styling for all form components to set the `text-disabled` class on the label and hint elements.
3 changes: 2 additions & 1 deletion packages/ember-toucan-core/src/-private/components/hint.hbs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<div
class="type-xs-tight text-body-and-labels mt-0.5"
class="type-xs-tight mt-0.5
{{if @isDisabled 'text-disabled' 'text-body-and-labels'}}"
...attributes
>{{yield}}</div>
7 changes: 6 additions & 1 deletion packages/ember-toucan-core/src/-private/components/hint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ import templateOnlyComponent from '@ember/component/template-only';

export interface ToucanFormHintComponentSignature {
Element: HTMLDivElement;
Args: {};
Args: {
/**
* Sets disabled styling on the hint.
*/
isDisabled?: boolean;
};
Blocks: {
default: [];
};
Expand Down
6 changes: 5 additions & 1 deletion packages/ember-toucan-core/src/-private/components/label.hbs
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
<label class="type-md-tight text-body-and-labels block" ...attributes>
<label
class="type-md-tight block
{{if @isDisabled 'text-disabled' 'text-body-and-labels'}}"
...attributes
>
{{yield}}
</label>
7 changes: 6 additions & 1 deletion packages/ember-toucan-core/src/-private/components/label.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@ import templateOnlyComponent from '@ember/component/template-only';

export interface ToucanFormLabelComponentSignature {
Element: HTMLLabelElement;
Args: {};
Args: {
/**
* Sets disabled styling on the label.
*/
isDisabled?: boolean;
};
Blocks: {
default: [];
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
)
}}
<legend
class="type-md-tight text-body-and-labels flex items-center gap-1.5"
class="type-md-tight flex items-center gap-1.5
{{if @isDisabled 'text-disabled' 'text-body-and-labels'}}"
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 may be tempting to make a GroupLegend component or something, that's essentially Label but renders a <legend and can be used here and with the radio-group; however, I punted on this for now. Happy to do it in a follow up if folks think it'd be worth it though.

data-label
>
{{#if (has-block "label")}}
Expand All @@ -36,7 +37,7 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint id={{field.hintId}} data-hint>
<field.Hint id={{field.hintId}} data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint data-hint>
<field.Hint data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
data-root-field={{if @rootTestSelector @rootTestSelector}}
>
<Form::Field as |field|>
<field.Label for={{field.id}}>
<field.Label for={{field.id}} @isDisabled={{@isDisabled}}>
{{#if
(this.assertBlockOrArgumentExists
(hash
Expand Down Expand Up @@ -32,7 +32,7 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint data-hint>
<field.Hint data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
class="flex items-center gap-1.5"
for={{field.id}}
data-label
@isDisabled={{@isDisabled}}
>
{{#if (has-block "label")}}
{{yield to="label"}}
Expand All @@ -35,14 +36,15 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint id={{field.hintId}} data-hint>
<field.Hint id={{field.hintId}} data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
{{@hint}}
{{/if}}
</field.Hint>
{{/if}}

<field.Control class="mt-1.5 flex">
<Form::Controls::Input
id={{field.id}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
)
}}
<legend
class="type-md-tight text-body-and-labels flex items-center gap-1.5"
class="type-md-tight flex items-center gap-1.5
{{if @isDisabled 'text-disabled' 'text-body-and-labels'}}"
data-label
>
{{#if (has-block "label")}}
Expand All @@ -39,7 +40,7 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint id={{field.hintId}} data-hint>
<field.Hint id={{field.hintId}} data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint data-hint>
<field.Hint data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
class="flex items-center gap-1.5"
for={{field.id}}
data-label
@isDisabled={{@isDisabled}}
>
{{#if (has-block "label")}}
{{yield to="label"}}
Expand All @@ -35,7 +36,7 @@
(hash blockExists=(has-block "hint") argName="hint" arg=@hint)
)
}}
<field.Hint id={{field.hintId}} data-hint>
<field.Hint id={{field.hintId}} data-hint @isDisabled={{@isDisabled}}>
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{else}}
Expand Down
3 changes: 3 additions & 0 deletions test-app/tests/integration/components/checkbox-field-test.gts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module('Integration | Component | Fields | CheckboxField', function (hooks) {
</template>);

assert.dom('[data-label]').hasText('Label');
assert.dom('[data-label]').hasClass('text-titles-and-attributes');
Copy link
Contributor Author

@ynotdraw ynotdraw Jun 28, 2023

Choose a reason for hiding this comment

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

I've got mixed feelings on asserting class names in tests, as I'd prefer visual regression testing instead; however, I want to make sure we don't regress until we get something like Percy added.

Note 2: Radio and checkbox labels use text-titles-and-attributes instead of text-body-and-labels intentionally by design


assert
.dom('[data-hint]')
Expand Down Expand Up @@ -122,6 +123,8 @@ module('Integration | Component | Fields | CheckboxField', function (hooks) {
assert.dom('[data-checkbox]').isDisabled();

assert.dom('[data-lock-icon]').exists();

assert.dom('[data-label]').hasClass('text-disabled');
});

test('it sets readonly on the checkbox using `@isReadOnly` and renders a lock icon', async function (assert) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module('Integration | Component | Fields | CheckboxGroup', function (hooks) {
assert.dom('[data-group-field]').hasNoAttribute('aria-invalid');

assert.dom('[data-label]').hasText('Label');
assert.dom('[data-label]').hasClass('text-body-and-labels');

assert.dom('[data-lock-icon]').doesNotExist();
});
Expand Down Expand Up @@ -180,6 +181,8 @@ module('Integration | Component | Fields | CheckboxGroup', function (hooks) {
assert.dom('[data-checkbox-2]').isDisabled();

assert.dom('[data-lock-icon]').exists();

assert.dom('[data-label]').hasClass('text-disabled');
});

test('it sets an individual checkbox to disabled with `@isDisabled`', async function (assert) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ module('Integration | Component | Fields | FileInput', function (hooks) {
</template>);

assert.dom('[data-label]').hasText('Label');
assert.dom('label').hasClass('text-body-and-labels');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FileInput is a bit different than the others, where because of the markup, data-label is actually a nested element within <label. Due to that, I'm targeting the label tag directly here instead of using [data-label]


assert.dom('[data-trigger]').hasText('Select Files');

assert
Expand Down Expand Up @@ -192,6 +194,8 @@ module('Integration | Component | Fields | FileInput', function (hooks) {
assert.dom('[data-file-input-field]').hasClass('text-disabled');

assert.dom('[data-lock-icon]').exists();

assert.dom('label').hasClass('text-disabled');
});

test('it sets readonly on the input using `@isReadOnly` and renders a lock icon', async function (assert) {
Expand Down
4 changes: 4 additions & 0 deletions test-app/tests/integration/components/input-field-test.gts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ module('Integration | Component | Fields | Input', function (hooks) {
assert.dom(label).exists('Expected to have label block rendered');
assert.dom(label).hasText('Label', 'Expected to have label text "label"');
assert.dom(label).hasAttribute('for', inputId);
assert.dom(label).hasClass('text-body-and-labels');

assert.dom(input).exists('Expected to have input tag rendered');
assert.dom(input).hasAttribute('type', 'text');
assert.dom(input).hasAttribute('id');
Expand Down Expand Up @@ -226,6 +228,8 @@ module('Integration | Component | Fields | Input', function (hooks) {
assert.dom('[data-input]').isDisabled();

assert.dom('[data-lock-icon]').exists();

assert.dom('[data-label]').hasClass('text-disabled');
});

test('it sets readonly on the input using `@isReadOnly` and renders a lock icon', async function (assert) {
Expand Down
3 changes: 3 additions & 0 deletions test-app/tests/integration/components/radio-field-test.gts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ module('Integration | Component | Fields | Radio', function (hooks) {
</template>);

assert.dom('[data-label]').hasText('Label');
assert.dom('[data-label]').hasClass('text-titles-and-attributes');

assert
.dom('[data-hint]')
Expand Down Expand Up @@ -88,6 +89,8 @@ module('Integration | Component | Fields | Radio', function (hooks) {
</template>);

assert.dom('[data-radio]').isDisabled();

assert.dom('[data-label]').hasClass('text-disabled');
});

test('it sets readonly on the radio using `@isReadOnly`', async function (assert) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ module('Integration | Component | Fields | RadioGroup', function (hooks) {
assert.dom('[data-group-field]').hasAttribute('aria-required');

assert.dom('[data-label]').hasText('Label');
assert.dom('[data-label]').hasClass('text-body-and-labels');

assert.dom('[data-lock-icon]').doesNotExist();
});
Expand Down Expand Up @@ -119,6 +120,8 @@ module('Integration | Component | Fields | RadioGroup', function (hooks) {
assert.dom('[data-radio-2]').isDisabled();

assert.dom('[data-lock-icon]').exists();

assert.dom('[data-group-field] [data-label]').hasClass('text-disabled');
});

test('it sets an individual radio to disabled with `@isDisabled`', async function (assert) {
Expand Down
3 changes: 3 additions & 0 deletions test-app/tests/integration/components/textarea-field-test.gts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module('Integration | Component | Fields | Textarea', function (hooks) {
</template>);

assert.dom('[data-label]').hasText('Label');
assert.dom('[data-label]').hasClass('text-body-and-labels');

assert
.dom('[data-hint]')
Expand Down Expand Up @@ -130,6 +131,8 @@ module('Integration | Component | Fields | Textarea', function (hooks) {
assert.dom('[data-textarea]').hasClass('text-disabled');

assert.dom('[data-lock-icon]').exists();

assert.dom('[data-label]').hasClass('text-disabled');
});

test('it sets readonly on the textarea using `@isReadOnly` and renders a lock icon', async function (assert) {
Expand Down