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

Conversation

@ynotdraw ynotdraw self-assigned this Jun 27, 2023
@changeset-bot
Copy link

changeset-bot bot commented Jun 27, 2023

🦋 Changeset detected

Latest commit: cf944f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@crowdstrike/ember-toucan-core Patch
@crowdstrike/ember-toucan-form Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 27, 2023

Preview URLs

Env: preview
Docs: https://d442af81.ember-toucan-core.pages.dev

@@ -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.

@@ -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

@@ -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]

Base automatically changed from improve-dx-w-pnpm-sync to main June 29, 2023 12:41
@ynotdraw ynotdraw merged commit 1588e2b into main Jun 29, 2023
16 checks passed
@ynotdraw ynotdraw deleted the fix-disabled-text-styles branch June 29, 2023 19:05
@github-actions github-actions bot mentioned this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants