Skip to content

Conversation

@mjauvin
Copy link
Member

@mjauvin mjauvin commented Apr 29, 2023

No description provided.

@mjauvin mjauvin added the maintenance PRs that fix bugs, are translation changes or make only minor changes label Apr 29, 2023
@mjauvin mjauvin added this to the v1.2.3 milestone Apr 29, 2023
@mjauvin mjauvin self-assigned this Apr 29, 2023
@bennothommo
Copy link
Member

bennothommo commented Apr 30, 2023

Just a thought - should we even call the extendFields events for nested forms? isNested is only true for nested form and repeater widgets, and it stands to reason that the user can still extend the primary form to add fields to these widgets, if necessary.

@mjauvin
Copy link
Member Author

mjauvin commented Apr 30, 2023

Just a thought - should we even call the extendFields events for nested forms? isNested is only true for nested form and repeater widgets, and it stands to reason that the user can still extend the primary form to add fields to these widgets, if necessary.

How would that work exactly? The user would get the nested/repeater widgets from the main formWidget and extend its fields directly ?

@LukeTowers
Copy link
Member

Just a thought - should we even call the extendFields events for nested forms? isNested is only true for nested form and repeater widgets, and it stands to reason that the user can still extend the primary form to add fields to these widgets, if necessary.

That seems like a can of worms I don't want to open.

@bennothommo
Copy link
Member

bennothommo commented May 1, 2023

This requirement for checking isNested has tripped up a few people now, and I don't think it's a good experience for form changes to spill into the nested forms unless its explicit, nor is it ideal that we even have to document this whole "you need to add some condition checks on isNested if you don't want your nested forms to be affected."

@LukeTowers
Copy link
Member

The problem is how people are attaching the listener. The global event fires at all times on all instances of a Form widget and it must stay that way regardless of whether or not a Form instance is nested.

It is already standard practice to check for the model class match, the controller class match, and the isNested call. I would recommend that we just provide a better example in the code example section rather than having a separate call out specifically for the isNested property.

@mjauvin mjauvin changed the title Add a note about $formWidget->isNested in extendFields events Add example for $formWidget->isNested in extendFields events May 1, 2023
@mjauvin
Copy link
Member Author

mjauvin commented May 1, 2023

Done @LukeTowers

@LukeTowers LukeTowers changed the title Add example for $formWidget->isNested in extendFields events Improve docs for checking for the correct Form widget instance May 2, 2023
@LukeTowers LukeTowers merged commit fc74832 into develop May 2, 2023
@LukeTowers LukeTowers deleted the add-isnested-note branch May 2, 2023 23:36
@mjauvin
Copy link
Member Author

mjauvin commented May 16, 2023

@bennothommo @LukeTowers should that be moved to 1.2.2 since it has been merged already?

@bennothommo bennothommo modified the milestones: v1.2.3, v1.2.2 May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance PRs that fix bugs, are translation changes or make only minor changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants