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

Fixed missing use statement in FormCollectionElementGroups. #2159

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

Daniel-KM
Copy link
Contributor

No description provided.

@zerocrates
Copy link
Member

Have you tried this out with the fix and a "grouped" element that's in a fieldset? It's definitely a bug, but I have a suspicion that supporting elements that are in fieldsets wouldn't work very well anyway: it seems to me that it would probably render the element twice: once in the group and once in the fieldset.

We might instead want to just remove the code that tries to walk down into fieldsets entirely, rather than fixing the use.

@Daniel-KM
Copy link
Contributor Author

This is a bug i see when trying module bulk edit, that adds many fieldsets to the batch form.

@Daniel-KM
Copy link
Contributor Author

Yes, the group of omeka matches the standard fieldset of html and laminas, but without adding a level in names. So for now, i fixed the new behavior in my modules to avoid to remove fieldsets.

@zerocrates
Copy link
Member

OK, and I've just gone ahead and tested with a simple fieldset and it's definitely preferable to have it correctly descend into the fieldsets in groupElements, otherwise we're just getting completely broken mis-rendering of the fieldset as an "element" once there are defined groups. So this is definitely a straightforward improvement.

@zerocrates zerocrates merged commit dcab4c1 into omeka:develop Apr 4, 2024
5 checks passed
@Daniel-KM Daniel-KM deleted the fix/form_groups branch April 4, 2024 20:01
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