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

implements container.formGroupClass parameter #403

Closed
wants to merge 6 commits into from

Conversation

skie
Copy link
Contributor

@skie skie commented Nov 16, 2024

Implements container.formGroupClass parameter to pass or disable classes different from form-group to containers.

When defined complex fields that should have their own containers, now possible to assign the .form-group class at wrapper level, while the inner fields rendered by FriendsOfCake/bootstrap-ui would not have it.

unset($options['container']['formGroupClass']);
if (empty($options['container'])) {
unset($options['container']);
}
}

$containerOptions = $options['container'];
Copy link
Member

Choose a reason for hiding this comment

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

This line will generate an undefined index error if the unset() on line 670 is executed. Test case for when $skipFormGroupClass ends up being true is needed.

P.S. I have fixed the test suite issues on upstream master, so you can rebase.

@ADmad
Copy link
Member

ADmad commented Nov 16, 2024

I am a bit unsure about these approach. This increases the if/else conditions and makes the code more difficult to follow/maintain in the long run. The idea behind using string templates was avoid this problem.

Instead of appending the formGroupClass to containerClass, how about replacing the hardcoded form-group in the templates, with a new template var formGroupClass? Default it to form-group and override with user provided value if required. I am pretty sure that will require minimal code changes.

@ADmad ADmad changed the base branch from master to next November 16, 2024 17:20
@skie
Copy link
Contributor Author

skie commented Nov 17, 2024

new approach in separate pull requiest #406

@skie skie closed this Nov 22, 2024
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