Form row component experiment

[snipe]

This is a bit of an experiment on my part, an attempt to solve a few problems at once:

1. Make sure the HTML is consistent throughout for all of the forms (this includes sizes, additional styles or classes, etc)
2. Remove the CSS classes and styles from the form partials so we can more easily change CSS frameworks or upgrade existing ones
3. Make sure all text fields have maxlength included

We have several spots in the (huge) codebase where forms look slightly different. Sometimes the labels are aligned to the left, sometimes to the right, sometimes they're bold, sometimes they're not, etc.

By abstracting this out into cascading blade components, we would be able to change the classes (assuming we have very few overrides) for all forms very quickly.

In the example in the image below:

That blade component code would look like this:

<!-- Name -->
    <x-form-row
            :label="trans('admin/manufacturers/table.name')"
            :item="$item"
            :errors="$errors ?? null"
            name="name"
            input_style="width: 50%"
            class="test"
            label_style="color: red"
            div_style="background-color: red"
            placeholder="this is some text"
            input_div_class="col-md-6 col-sm-12"
    />

The HTML rendered from that is:

 <!-- Name -->
    <!-- form-row blade component -->

    <div class="form-group test">

        <!-- form-label blade component -->

        <label class="control-label col-md-3 col-sm-12 col-xs-12" style="color: red;" for="name">
            Name
        </label>

        <div class="col-md-6 col-sm-12 test" style="background-color: red" class="test">

            <!-- input-text blade component -->
            <input class="form-control" aria-label="name" name="name" placeholder="this is some text" id="name" value="Microsoft" type="text" style="width: 50%" aria-label="name" name="name" placeholder="this is some text" id="name" value="Microsoft" type="text" required/>

        </div>
    </div>

I've tried to account for all of the "extras" we might run into (styles, placeholders, error text, help text, etc). A few more bits to handle, but I did want to see if I was going in a direction the team feels makes sense.

Oh, also, if the style passed is null, I'm still seeing that stupid style=";" being injected. Not sure how to fix that one. It doesn't actually hurt anything, it's just dumb.

Remaining

• Handle input groups
• Merge the date pickers
• Handle checkboxes + radios (their labels behave differently)
• Handle the select lists 😬 Not looking forward to that tbh

[uberbrady]

I think this looks great - I love stripping out all of that repetitive boilerplate HTML and simplifying it so much - I think that's going to be awesome! I had a few notes I added, but I don't think anything fatal - this looks like a great start to componentizing us in more and more places! Can't wait to see what the next one brings :)

[uberbrady]

It's a stylistic thing, but you could use $slot here - making your labels look something like:

<x-label.whatever blah="blah" blah="blah">
    This is the actual Label
</x-label.whatever>

[snipe]

I did it this way to ensure that we have control-label and whatever other styles might be needed. Possible I'm misunderstanding though.

[marcusmoore]

I think he means removing @props and swapping {{ $label }} for {{ $slot }}. The attributes will still be merged (class="control-label" will still render) and the calling syntax would change from

<x-form-label
    :$label
    :for="$name"
    :style="$label_style ?? null"
    class="{{ $label_class }}"
/>

to

<x-form-label
    :for="$name"
    :style="$label_style ?? null"
    class="{{ $label_class }}"
>
    {{ $label }}
</x-form-label>

It's not a big difference but it's worth noting the attributes are still merged using that syntax.

Personally, I like the "content" to be passed via $slot because it feels more natural than passing it as an attribute but either way works for me.

[snipe]

That gives me Undefined variable $label

[snipe]

When I swap it for {{ $slot }}, it works, but for such a simple blade, I'm not really sure how that helps us.

[marcusmoore]

$slot feels more natural to me in this scenario but either way works.

[snipe]

Sure, but that screws up the flexibility I just built into the checkboxes.

[marcusmoore]

Gotcha. I hadn't gotten that far into my review 😅

[snipe]

Okay, just a quick update for that last push.

I've now got something in place to handle checkboxes, although it will only handle one per line. (That's generally how this stuff works in our app anyway, so it's fine.)

Because of the various layouts we might use, we might want to hide the left-side column and have the label be next to the checkbox on the right instead.

In the image below (ignore the help text, I was just testing), you can see the difference.

<!-- Reassignable -->
    <x-form-row-checkbox
            :label="trans('admin/licenses/form.reassignable')"
            :$item
            :$errors
            name="reassignable"
            :value_text="trans('general.yes')"
    />

    <!-- Reassignable -->
    <x-form-row-checkbox
            :$item
            :$errors
            :value_text="trans('admin/licenses/form.reassignable')"
            name="reassignable"
    />

For the normal (label on the left) view, I netted out with:

<!-- Maintained -->
    <x-form-row-checkbox
            :label="trans('admin/licenses/form.maintained')"
            :$item
            :$errors
            :value_text="trans('general.yes')"
            name="maintained"
            checkbox_value="1"
    />

And to make it disabled:

<!-- Maintained -->
    <x-form-row-checkbox
            :label="trans('admin/licenses/form.maintained')"
            :$item
            :$errors
            :value_text="trans('general.yes')"
            name="maintained"
            disabled="true"
            checkbox_value="1"
    />

[marcusmoore]

I still have more files to check out but here are some readability improvements.

Overall this looks pretty good and is a big step in the right direction 😄

[marcusmoore]

Gotcha. I hadn't gotten that far into my review 😅

[marcusmoore]

Having a $attributes->merge(['class' => on the parent div and the input makes me 🤔

I noticed the same on form-row itself. I'm side-eyeing it but it seems to work fine.

[snipe]

As a side note, I need to revisit https://technology.blog.gov.uk/2020/02/24/why-the-gov-uk-design-system-team-changed-the-input-type-for-numbers/ to determine if we really want to use the number input type. :-/ It's possible browsers handle this a little better these days.