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

Proper formatting of checkbox for bootstrap 5 #393

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Jan 12, 2025

Currently check boxes display horribly. The main issue is with the <f:all tag not having a clean way of passing styling attributes to specific widgets.

This PR allows:

<f:all bean="${propertyName}" class="row" requiredClass="mb-3 required" labelClass="col-sm-2 col-form-label text-sm-end" divClass="col-sm-10" widget-selectDateClass="w-auto form-select d-inline" widget-class="form-control" widget-checkBoxClass="form-check-input align-middle"  widget-invalidClass="is-invalid" />

Specifically

widget-selectDateClass="w-auto form-select d-inline"  widget-checkBoxClass="form-check-input align-middle"

Does 2 things.

  1. removes selectDateClass from attrs for other widgets.
  2. Allows passing of classes necessary to format check boxes correctly.

Should later be refined via #392

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

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

Isn't it a core design goal of fields to move this type of logic into templates?

If you need a default check box, you write a checkbox specific template and read any widget attrs.

This change looks like we're wanting to avoid writing templates and introduce logic directly into the default form generation.

If we're wanting the benefits of fields (pulling constraints & properties from beans) to setup a form tag, we really should split out that logic separate from the template logic. Otherwise, we'll play wack-o-mole on adding attributes.

As this is, I'm not for this change since the defaults are meant to be catch alls, and not specific to a given theme.

@codeconsole
Copy link
Contributor Author

codeconsole commented Jan 13, 2025

Isn't it a core design goal of fields to move this type of logic into templates?

If that was the core design goal, why does it have an <f:all tag which is being used heavily by the scaffolding plugin and custom class names would never have been used.

@jdaugherty so what do you propose, leave it broken and poorly formatted? That is the way it currently is.
Or recommend not using Dates or booleans?

Previously it depended on a custom css file. This at least allows the flexibility to keep the old way or use bootstrap 5 without having to depend on custom css.

<~- create -->
<f:all bean="${propertyName}" class="row" requiredClass="mb-3 required" labelClass="col-sm-2 col-form-label text-sm-end" divClass="col-sm-10" widget-class="form-control" widget-invalidClass="is-invalid" />

<~- edit -->
<f:all bean="${propertyName}" class="row" requiredClass="mb-3 required" labelClass="col-sm-2 col-form-label text-sm-end" divClass="col-sm-10" widget-selectDateClass="w-auto form-select d-inline" widget-class="form-control" widget-checkBoxClass="form-check-input align-middle"  widget-invalidClass="is-invalid" />

<-- index -->
<f:table class="scaffold table table-striped table-sm" controller="\${controllerName}" collection="\${${propertyName}List}"/>

<-- show -->
<f:display bean="sample" listClass="container" listItemClass="row mb-3" labelClass="form-label col-sm-3 text-sm-end" valueClass="col-sm-9" />

Currently, I do use templates to make this work, but the default implementation should not require templates and at least render readable.

Also note that this is an intermediate solution with a TODO reference to an open Issue to iteratively refine this. This is just a work around to get things working correctly.

Copy link
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

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

I'll take my feedback to the ticket and will approve as an intermediate solution.

@codeconsole codeconsole merged commit ca9fbb8 into gpc:6.0.x Jan 14, 2025
2 checks passed
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