Form Components #62
Form Components #62
Conversation
There was a problem hiding this comment.
This is a great addition, Rich!
I have a couple ideas for consideration (see below). Also, I think it would be good to add a link to COMPONENTS.md in the main README.md next to the Libraries section and similarly brief.
I still need to do testing, but I don't expect to get to that right away, so just commenting on this for now.
There was a problem hiding this comment.
Rich, I found a few issues while making a demo page with the form components (I still need to review dialogs). I've commented on items that were blockers to what appeared to be needed or intended functionality.
These came up as I built this form: https://laravel-demo-test.hosting.cornell.edu/forms (it's a deploy of laravel-demo components branch).
- At the top of the page is what you had in the documentation.
- That is followed by blocks that have the CSS Framework reference I was trying to reproduce and the Starter Kit components implementation as best I understood from the docs + code.
I'd be interested in talking about how close to get to the CSS Framework code, since some differences may be by design and I'm just not aware.
There was a problem hiding this comment.
Good progress! I updated the components on my test site and tried them out. There were a few more issues, but most of them are pretty quick fixes. At this point I have tested text field variations, checkbox variations, and selects. Radios, buttons, and dialogs still to go.
| aria-describedby="{{ $field }}_desc" | ||
| @endif | ||
| /> | ||
| <span class="option-label" for="{{$field}}">{!!$text??''!!}</span> |
There was a problem hiding this comment.
| <span class="option-label" for="{{$field}}">{!!$text??''!!}</span> | |
| <span class="option-label">{!! $text ?? $slot !!}</span> |
Two suggestions here:
- Remove the "for", because this is not a
<label>tag and the attribute has no meaning - Supplement
$textwith$slot. I think in this layout this content is probably required, so taking advantage of the slot style seems like a nice implementation to me. But it's just an idea.
Regardless of whether you want to use text or slot, it needs to be documented in the README example, because I only found it when I couldn't figure out why my checkbox didn't show anything.
| <x-cd.form.form-item field="{{ $name ?? $attributes->whereStartsWith('wire:model')->first() }}" | ||
| classes="{{ $classes ?? '' }}" | ||
| required="{{ (($required??'') === 'false') ? 0 : (boolval($required??'')?1:0) }}" | ||
| :description="$description ?? ''" | ||
| > |
There was a problem hiding this comment.
I think this usage of form-item for the checkboxes is creating more problems than it is solving, but I don't see a simple solution. Here are the issues when I try to create a list of checkboxes like the CSSF example:
- It nests the checkboxes as form-items inside a single form item
- It adds a label, but there is no element with the id it says it's for
- If a description is provided, it's placed after the list, which seems unlikely as a typical design pattern
I think the CSSF example would suggest just removing this wrapper entirely. But we lose the "required" feature and the automatic wiring for the description. It's easy enough to pull the description code up to this component. Not sure what to do for labeling and requiredness, though.
There was a problem hiding this comment.
I think other changes fix the form-items nesting; the trailing description is for consistency with other components. If you don't specify it, it doesn't appear. To fix the label issue, I think we need a div with the same styling as the label; I'll work on that.
| <fieldset class="{{$legendtype??'default'}}"> {{-- default or semantic --}} | ||
| <legend @if ($legend_sr_only??false == 'true') class="sr-only" @endif >{{$legend??'Form'}}</legend> | ||
| {{$slot}} | ||
| </fieldset> |
There was a problem hiding this comment.
Why is a fieldset part of the base form? It seems like the features of the fieldset here might be better abstracted to their own component and then if you need a fieldset you can use it?
There was a problem hiding this comment.
I believe this is a WA consideration that is discussed in on Tone's form example page.
There was a problem hiding this comment.
Yep, I've seen what you mention. That's a good reason for fieldset to be in a component. But I found that when I went to try to use a fieldset within <x-cd.form> that it was treated as a nested fieldset and I couldn't avoid it.
I'm not sure where the balance on this lies between ease of use of the x-cd.form component versus needing to implement work-arounds when it isn't doing what you need.
There was a problem hiding this comment.
I don't think it is optional.
There was a problem hiding this comment.
Do you have a specific reference describing that a fieldset for a entire form is required? It is required when there is a group of related fields. But Tone's documentation encourages not having nested fieldsets and avoiding wrapping a lot set of fields in a single fieldset. To follow these recommendations on anything other than simple forms, we would have to not use this base form component.
Co-authored-by: Eric Woods <[email protected]>
|
I was working on trying to use these and realized there isn't a textarea input. Seems like that should be in our standard collection? |
Co-authored-by: Eric Woods <[email protected]>
| <x-cd.form.checkbox label="Subscription" wire:model.live="subscribe"/> | ||
| Subscribe | ||
| </x-cd-form.checkbox> |
There was a problem hiding this comment.
| <x-cd.form.checkbox label="Subscription" wire:model.live="subscribe"/> | |
| Subscribe | |
| </x-cd-form.checkbox> | |
| <x-cd.form.checkbox label="Subscription" wire:model.live="subscribe"> | |
| Subscribe | |
| </x-cd.form.checkbox> |
Typo
| @@ -0,0 +1,181 @@ | |||
| # Form Components | |||
|
|
|||
| A set of form components provides support the the styled form elements in the CD Framework. | |||
There was a problem hiding this comment.
| A set of form components provides support the the styled form elements in the CD Framework. | |
| A set of form components provides support for the styled form elements in the CD Framework. |
Typo
| <x-cd.form.checkbox label="Subscription" text="Subscribe" | ||
| wire:model.live="subscribe" | ||
| /> |
There was a problem hiding this comment.
Either this example should include something like a value='1' attribute or cd.form.checkbox should default $value, because this as written results in an undefined variable.
| ``` | ||
| <x-cd.form.radios label="Radios" wire:model.live="radios" :radiobuttons="$radiooptions" /> | ||
| ``` | ||
| Using `inline=1` alows the radio buttons to be displayed inline with flexbox. |
There was a problem hiding this comment.
I couldn't find evidence of this feature actually existing, but it would be great to have!
| ``` | ||
| <x-cd.form.submit-button /> | ||
| <x-cd.form.reset-button /> | ||
| <x-cd.form.cancel-button wire:click"closemodal"> |
There was a problem hiding this comment.
| <x-cd.form.cancel-button wire:click"closemodal"> | |
| <x-cd.form.cancel-button wire:click="closemodal" /> |
Typo
| <x-slot name="field_title">{!! $label !!}</x-slot> | ||
| <div class="flex-grid compact-rows"> |
There was a problem hiding this comment.
What would you think of putting $slot between these:
| <x-slot name="field_title">{!! $label !!}</x-slot> | |
| <div class="flex-grid compact-rows"> | |
| <x-slot name="field_title">{!! $label !!}</x-slot> | |
| {!! $slot !!} | |
| <div class="flex-grid compact-rows"> |
That would make it possible to include content between the heading and the options, like in the "A Plethora of Checkboxes" example
Livewire Form Component library
This PR does the following
To Review: