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

Allow customization of default wrapper in order to support Bootstrap without requiring templates #349

Merged
merged 8 commits into from
Sep 15, 2024

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Sep 7, 2024

This Objective

Reduce the amount of code in generate-app to the least amount possible while still maintaining readability and structure.

It is here for review and feedback

  1. Modify <f:all /> - Complete
<f:all bean="domain" class="row mb-3" labelClass="col-sm-2 col-form-label text-sm-end" divClass="col-sm-10" invalidClass="" requiredClass="" widget-class="form-control" widget-invalidClass="is-invalid" />

Design Decisions: <li><label></label><div>widget</div></li> pattern is needed. To maintain backwards compatibility, the div is only injected if the divClass attribute is present.

  1. Modify <f:table collection="domainList"/>
<f:table class="table table-striped table-sm" controller="${controllerName}" collection="${sampleList}" />
  1. Modify <f:display bean="domain" />
<f:display bean="sample" class="container" listItemClass="row mb-3" labelClass="form-label col-sm-3 text-sm-end" valueClass="col-sm-9" />

@codeconsole
Copy link
Contributor Author

#348

Copy link
Member

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

All minor/trivial comments.

Tests for the new code are missing.

if (k?.startsWith(prefixAttribute))
widgetAttrs[k.replace(prefixAttribute, '')] = v
else
wrapperAttrs[k] = v
}

List classes = [widgetAttrs['class']?:'']
if (model.invalid) classes << (widgetAttrs.remove('invalidClass')?:'')
if (model.required) classes << (widgetAttrs.remove('requiredClass')?:'')
Copy link
Member

Choose a reason for hiding this comment

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

To follow the code-style (if any) of the existing code, add spaces around ?: (throughout changes)

Copy link
Member

@sbglasius sbglasius left a comment

Choose a reason for hiding this comment

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

All good

Copy link

@jamesfredley jamesfredley left a comment

Choose a reason for hiding this comment

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

Adding a few tests to exercise the new functionality would ensure it doesn't regress.

@codeconsole
Copy link
Contributor Author

@jamesfredley existing tests should verify functionality because all the new attributes have existing defaults. If the code changed any existing behavior, the unit tests would fail.

I am a little hesitant to add a unit tests for any new capabilities because, with consensus from everyone, I would like to completely change the default behavior for 7.0.

@jamesfredley
Copy link

@codeconsole ok, if it is all going to change, the test would be short lived and unneeded.

@codeconsole codeconsole merged commit 17beafa into gpc:master Sep 15, 2024
1 check 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.

3 participants