-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add labels and hint blocks to fields #110
Conversation
🦋 Changeset detectedLatest commit: 26b324f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Preview URLsEnv: preview |
72b72da
to
e42c891
Compare
2be4131
to
92e5d2a
Compare
e3971b1
to
4f62489
Compare
@@ -3,7 +3,10 @@ | |||
@label='Label' | |||
@isChecked={{this.isChecked}} | |||
@onChange={{this.handleChange}} | |||
/> | |||
> | |||
<:label>extra label info</:label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not feeling super imaginative here...if anyone has any suggestions of what to put in the blocks I will do a sweep and update the content in a single commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could maybe do something like what we are expecting to do - add an icon here (with no tooltip for now):
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" stroke="currentColor" viewBox="0 0 24 24"><path d="M12 3a9 9 0 11-6.364 2.636A8.972 8.972 0 0112 3zm0 4.7v5.2m0 3.39v.01" fill="none" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></path></svg>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wait...with these changes all these :named-blocks
should be removed from these demos.
Sorry for busting your PR again, but now I'm done moving things/renaming things! |
Nice to see things getting reorganized tho! |
class="type-md-tight text-body-and-labels block" | ||
data-label | ||
>{{@label}} {{yield to="label"}}</legend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question: do we really want to render both the argument and the block, in case both are provided? This is not wrong per se, but maybe confusing? We could instead also give one a higher priority, and render only that:
Arg has priority:
{{#if @label}}
{{@label}}
{{else}}
{{yield to="label"}}
{{/if}}
or block has priority:
{{#if (has-block "label")}}
{{yield to="label"}}
{{else}}
{{@label}}
{{/if}}
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I think rendering one or the other makes sense.
@ynotdraw thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree on choosing one or the other but not both! I'm leaning towards block having priority for no real reason other but have 0 strong opinions.
<Form::Field as |field|> | ||
<fieldset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding: what was the reason to not use <Form::Fieldset>
anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be able to put {{yield to="label"}}
and {{yield to="hint}}
inside of say, the block for field.Label
and the Fieldset
component does not yield these subcomponents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if something similar to what I'm thinking for the form/core wrapper may work here. I was hacking on this last night quite a bit and think I found something that'd work, although it's not necessarily pretty. I'm curious if @simonihmig knows if there's a way to conditionally render named blocks when you have two-levels of components (e.g., <Fieldset
exposes named blocks, but then another consuming component like CheckboxGroup
wants to expose those underlying named blocks as well).
You can't conditionally render named-blocks inside of a block, but you can conditionally render components. So something really ugly like 3ed2f97#diff-68aa5e5601f7498e493199aca7a867ff41712f515756a155c3f7dcf1ccd45a10 may give us the best of both worlds? Where folks can provide component arguments or named blocks or a combination of both. That's what I was hacking on last night and it seems to work, although it's definitely not the best looking thing in the world.
For this case I think it'd look something like:
Update Fieldset to yield the new blocks like you have in this PR.
<Form::Field as |field|>
<fieldset
aria-describedby="{{if @error field.errorId}} {{if @hint field.hintId}}"
disabled={{@isDisabled}}
...attributes
>
{{! Use your new assert thing here instead, but just lazily posting the idea }}
<legend
class="type-md-tight text-body-and-labels block"
data-label
>{{@label}}{{yield to="label"}}</legend>
{{! Use your new assert thing here instead, but just lazily posting the idea }}
{{#if @hint}}
<field.Hint id={{field.hintId}} data-hint>{{@hint}}{{yield to="hint"}}</field.Hint>
{{/if}}
<field.Control
class="mt-1.5 flex flex-col space-y-2 rounded-sm p-1
{{if @error 'shadow-error-outline'}}"
data-control
>
{{yield}}
</field.Control>
{{#if @error}}
<field.Error id={{field.errorId}} @error={{@error}} data-error />
{{/if}}
</fieldset>
</Form::Field>
And then in CheckboxGroup
, use helpers to determine which mix of component arguments or named blocks are being used. Map all properties over.
<Form::Fieldset
@label={{@label}}
@hint={{@hint}}
@error={{@error}}
@isDisabled={{@isDisabled}}
aria-invalid={{if @error "true"}}
...attributes
>
{{#if (has-block "label")}}
{{yield to="label"}}
{{/if}}
{{#if (has-block "hint")}}
{{yield to="hint"}}
{{/if}}
{{yield
(hash
CheckboxField=(component
(ensure-safe-component this.CheckboxFieldComponent)
name=@name
onChange=this.handleInput
isDisabled=@isDisabled
selectedValues=@value
)
)
}}
</Form::Fieldset>
✅ But you could do something like this: ✅
{{#if (this.hasOnlyLabelBlock (has-block "label") (has-block "hint"))}}
<Form::Fieldset
@hint={{@hint}}
@error={{@error}}
@isDisabled={{@isDisabled}}
aria-invalid={{if @error "true"}}
...attributes
>
<:label>{{yield to="label"}}</:label>
{{yield
(hash
CheckboxField=(component
(ensure-safe-component this.CheckboxFieldComponent)
name=@name
onChange=this.handleInput
isDisabled=@isDisabled
selectedValues=@value
)
)
}}
</Form::Fieldset>
{{else if (this.hasHintAndLabelBlocks (has-block "label") (has-block "hint"))}}
<Form::Fieldset
@hint={{@hint}}
@error={{@error}}
@isDisabled={{@isDisabled}}
aria-invalid={{if @error "true"}}
...attributes
>
<:label>{{yield to="label"}}</:label>
<:hint>{{yield to="hint"}}</:hint>
{{yield
(hash
CheckboxField=(component
(ensure-safe-component this.CheckboxFieldComponent)
name=@name
onChange=this.handleInput
isDisabled=@isDisabled
selectedValues=@value
)
)
}}
</Form::Fieldset>
{{else if (this.hasLabelArgAndHintBlock @label (has-block "hint"))}}
<Form::Fieldset
@label={{@label}}
@hint={{@hint}}
@error={{@error}}
@isDisabled={{@isDisabled}}
aria-invalid={{if @error "true"}}
...attributes
>
<:hint>{{yield to="hint"}}</:hint>
{{yield
(hash
CheckboxField=(component
(ensure-safe-component this.CheckboxFieldComponent)
name=@name
onChange=this.handleInput
isDisabled=@isDisabled
selectedValues=@value
)
)
}}
</Form::Fieldset>
{{else}}
<Form::Fieldset
@label={{@label}}
@hint={{@hint}}
@error={{@error}}
@isDisabled={{@isDisabled}}
aria-invalid={{if @error "true"}}
...attributes
>
{{yield
(hash
CheckboxField=(component
(ensure-safe-component this.CheckboxFieldComponent)
name=@name
onChange=this.handleInput
isDisabled=@isDisabled
selectedValues=@value
)
)
}}
</Form::Fieldset>
{{/if}}
With your helpers here being:
hasOnlyLabelBlock = (hasLabel: boolean, hasHint: boolean) =>
hasLabel && !hasHint;
hasHintAndLabelBlocks = (hasLabel: boolean, hasHint: boolean) =>
hasLabel && hasHint;
hasLabelArgAndHintBlock = (hasLabel: boolean, hasHint: boolean) =>
hasLabel && hasHint;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also hasHintArgAndLabelBlock
and hasHintArgAndLabelArg
and hasOnlyLabelArg
I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ You can't do this:⚠️
What exactly can't we do?
Your example had
{{#if (has-block "label")}}
{{yield to="label"}}
{{/if}}
We can't do this?
Or is it about something like this, which is not possible? 👇
{{#if (has-block "label")}}
<:label>
{{yield to="label"}}
</:label>
{{/if}}
If that's the case, what is happening there? Does Ember refuse using any <:some-block>
wrapped in {{#if}}
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes named blocks like :label
can't be wrapped in conditionals. (Sorry, yesterday I was struggling to remember the exact details, but this is why!)
RFC issue: emberjs/rfcs#735
4f62489
to
fb68fef
Compare
@@ -3,7 +3,10 @@ | |||
@label='Label' | |||
@isChecked={{this.isChecked}} | |||
@onChange={{this.handleChange}} | |||
/> | |||
> | |||
<:label>extra label info</:label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could maybe do something like what we are expecting to do - add an icon here (with no tooltip for now):
<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" stroke="currentColor" viewBox="0 0 24 24"><path d="M12 3a9 9 0 11-6.364 2.636A8.972 8.972 0 0112 3zm0 4.7v5.2m0 3.39v.01" fill="none" stroke-linecap="round" stroke-linejoin="round" stroke-width="2"></path></svg>
<:default as |group|> | ||
<group.CheckboxField @label='Option 1' @value='option-1' /> | ||
<group.CheckboxField | ||
@label='Option 2' | ||
@hint='Extra information about the radio' | ||
@value='option-2' | ||
/> | ||
<group.CheckboxField @label='Option 3' @value='option-3' /> | ||
<group.CheckboxField | ||
@label='Option 4' | ||
@hint='Extra information about the radio' | ||
@value='option-4' | ||
/> | ||
</:default> | ||
<:label>extra label info</:label> | ||
<:hint>extra hint info</:hint> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering:
Should the named-block options go under a new section + under the UI states sections of the docs instead? I feel like they could use some extra documentation and that'd be a good way to show usage + what they look like visually. I kind of think that maybe the top-level demo can be a general use case, while maybe more "advanced" uses like this could go a bit further down the page. What do you think?
We'll also want to add documentation for these named blocks on all pages affected so that folks know they can use these!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These :label
and :hint
blocks are removed from this demo now (they should not have been here).
I also added :label
code snippets under each ## Label
section, same for :hint
. I don't think they can go under UI states because folks can't see the code there...I guess they could see an icon for label or a link for hint? Seems more useful to have it under the ## Label
and ## Hint
sections tho (in terms of being able to copy those code snippets for consumers)
aria-describedby="{{if @error field.errorId}} {{if @hint field.hintId}}" | ||
disabled={{@isDisabled}} | ||
{{! attribute aria-invalid is not supported by the element fieldset with the implicit role of group }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - so no more aria-invalid={{if @error "true"}}
due to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I guess the linter (ember-template-lint?) doesn't catch this when we were adding it as ...attributes
on a component, but now that it can tell it's a group it doesn't want you to add aria-invalid
?
packages/ember-toucan-core/src/components/form/fields/checkbox.ts
Outdated
Show resolved
Hide resolved
test-app/tests/integration/components/file-input-field-test.gts
Outdated
Show resolved
Hide resolved
test-app/tests/integration/components/file-input-field-test.gts
Outdated
Show resolved
Hide resolved
|
||
assert.dom(label).hasText('Label Extra label content'); | ||
assert.dom(hint).hasText('Hint text visible here Extra hint content'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is this spacing prettier would complain about? May want to save in VSCode to make sure prettier runs since it doesn't run via the CLI with gts
|gjs
files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay will give it a shot...whenever I open a gts file in my VS Code there are red lines everywhere so I just sort of assumed it wouldn't be formatting correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update - I think this did something! Will keep formatting markdown
and gts/gjs
files in VS Code from now on...
e44fe00
to
ffbad8b
Compare
packages/ember-toucan-core/src/components/form/fields/checkbox.hbs
Outdated
Show resolved
Hide resolved
packages/ember-toucan-core/src/components/form/fields/checkbox.hbs
Outdated
Show resolved
Hide resolved
ffbad8b
to
5cddee9
Compare
@@ -0,0 +1,39 @@ | |||
import { assert } from '@ember/debug'; | |||
|
|||
export type AssertBlockOrArg = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ynotdraw new singular function / not a helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay! This looks awesome to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming together really well! I think just a few docs / nit things and then it'll be good to go! 🚀
I also started playing around with this a bit for our next Form component on how we'll expose these named blocks over at 3ed2f97. We should chat about this on Monday! I'm so excited about this! Thanks for knocking this out!
Provide a string to `@label` to render the text into the `<legend>` of the fieldset. | ||
Required. | ||
|
||
Use either `@label` or `:label`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: To maybe be "modern Ember" friendly or more friendly to folks new to Ember, I wonder if being a bit more verbose here would be helpful:
Use either `@label` or `:label`. | |
Use either the `@label` component argument or the `:label` named block. |
|
||
Use either `@label` or `:label`. | ||
|
||
Provide a string to `@label` or components to `:label` to render the text into the `<legend>` of the fieldset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the :label
named block can really take any content!
Provide a string to `@label` or components to `:label` to render the text into the `<legend>` of the fieldset. | |
Provide a string to the `@label` component argument or content to the `:label` named block to render the into the `<legend>` of the fieldset. |
<p class='text-body-and-labels text-xs m-0 italic'>~Fieldset components render | ||
here!~</p> | ||
</Form::Fieldset> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to axe this?
docs/components/fieldset/index.md
Outdated
Use either `@label` or `:label`. | ||
|
||
Provide a string to `@label` or components to `:label` to render the text into the `<legend>` of the Fieldset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments here (and all the other files!) as above, but this is looking good!
Use either `@hint` or `:hint`. | ||
|
||
Provide a string to `@hint` or components to `:hint` to render the text into the Hint section of the fieldset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comments in this section as the above nit, but these are also just me being nit-picky!
error?: ErrorMessage; | ||
files?: File[]; | ||
hint?: string; | ||
label: string; | ||
label?: string; | ||
trigger: string; | ||
isDisabled?: boolean; | ||
multiple?: boolean; | ||
onChange?: (files: File[], event: FileEvent) => void; | ||
rootTestSelector?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside / not part of this PR / as a separate PR: it may be good to add JSDocs here like with our other components so that they show up in your editor!
{{@label}} | ||
{{/if}} | ||
</span> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
superrrr nit: looks like some extra spacing here that's not in the other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra line space should be removed now...I notice that this isn't fixed with pnpm format
which is a bit of a bummer.
}: AssertBlockOrArg) => { | ||
if (isRequired) { | ||
assert( | ||
`You need either :${argName} or @${argName}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Do you think we should put @${argName}
first to prompt folks to use the argument rather than the named block? Or do you think more people will use the named block version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure...
test-app/tests/integration/components/radio-group-field-test.gts
Outdated
Show resolved
Hide resolved
- change label and hint block from unknown to [] for TS - added tests for label and hint blocks
- added more code samples in as `:default` is sometimes required (when component yields components) - Label is required, hint is not
- assert checks if there is an arg or a block, and if it's required - assert warns user if they are trying to use both - has is a check to render only if a block or an arg exists (@Label versus :label)
The two helpers were doing pretty much the same thing (checking for a block or arg, or the presence of either if required). So I made this into a single function instead, and not a helper. It's not really a helper because it doesn't use the helper function.
Having a single helper helps to declutter the templates a bit.
This was breaking the sidebar navigation because an Error was being thrown on this page (assert Error).
- amend Label description with named blocks and component arg - amend Hint description with named blocks and component arg - add UI states for hint and label blocks - Add back in missing default block in the fieldset demo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Questions still around Fieldset
though from #110 (comment). I think we can figure this out later as it seems a bit tricky due to the way named blocks work.
I also wonder if we should delete Fieldset
as it's not being used at all with these changes and it'll be one more component to maintain that we're not sure if folks are going to use? I'd imagine most folks will use our checkbox-group and radio-group components alone rather than rolling their own, but who knows. If we could use the Fieldset
component for both of those cases, then I'd say let's keep it, but since we cannot due to the limitations mentioned in the those comments, I'm quite tempted to say we remove it for now (can always add it back later!).
NOTE: It may be wise to add a changeset with this PR as well! Then we can cut releases more frequently 🎉. If you do end up deleting Fieldset
it's probably worth selecting minor
as the upgrade (as it'll technically be a breaking change in pre1.0.0 land). Otherwise a patch
is probably fine since we are pre1.0.0 and it's new functionality with no breaking changes.
906961f
to
913681a
Compare
- remove fieldset test - remove fieldset demos and docs - remove fieldset from template registry - remove fieldset from package.json
913681a
to
26b324f
Compare
@@ -128,7 +128,6 @@ | |||
"./components/form/fields/radio-group.js": "./dist/_app_/components/form/fields/radio-group.js", | |||
"./components/form/fields/radio.js": "./dist/_app_/components/form/fields/radio.js", | |||
"./components/form/fields/textarea.js": "./dist/_app_/components/form/fields/textarea.js", | |||
"./components/form/fieldset.js": "./dist/_app_/components/form/fieldset.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋
🚀 Description
Added
label
andhint
named blocks to field components.label
andhint
named blocks to field componentslabel
andhint
to fieldset, radio group field, and checkbox group field componentsaria-invalid
from the checkbox field group, due to this lint rule: https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-unsupported-role-attributes.mdaria-invalid
Component | Fields | { Component Name }
@label
or:label
but not both at the same time@hint
or:hint
but not both at the same time@label
and:label
are missing, error because a label is required (either as a block or arg):hint
and:label
block@label
is provided@label
and:label
are provided:named-block
implementation forLabel
andHint
Fieldset
and related tests🔬 How to Test
:hint
and:label
have been added: https://dc754599.ember-toucan-core.pages.dev/docs/components/file-input-field📸 Images/Videos of Functionality