search

unclecheese / silverstripe-display-logic

The Display Logic module allows you to add conditions for displaying or hiding certain form fields based on client-side behavior.
BSD 3-Clause "New" or "Revised" License
74 stars 71 forks source link

FIX compatibility with lekoala/silverstripe-pure-modal #155

Closed gurucomkz closed 1 year ago

gurucomkz commented 1 year ago

Inputs in https://github.com/lekoala/silverstripe-pure-modal do not have "{FormID}_" prefix in holders/fields IDs. This patch detects pure-modal surrounding and uses different lookup selectors.

michalkleiner commented 1 year ago

Hi @gurucomkz, as much as we appreciate your PR, I don't think adding a support specific to/targeted at a single external module that potentially (haven't verified) does not wrap its form fields with a <form> markup (could you confirm this for us?) is the best way here. Is this perhaps fixable at the module's end by providing the form wrapper in a similar way to how SS default edit form does that? Alternatively we could support this in a generic way, perhaps providing an attribute to a wrapper on what parent element it needs to look for for the JS logic. What do you think?

gurucomkz commented 1 year ago

Hi @michalkleiner , thank you for your feedback.

The module doesn't care much about the enclosing form. It's merely adding the inputs that aren't attached to it (using setForm()) and perhaps that leads to the issue. That may have been done on purpose, though, but I don't know for sure.

Actually, I started this PR in attempt to begin a conversation about the issue and you may be totally right about the need to fix the other module instead. I'm just a user of both modules caught in between :) I will check with @lekoala about that

Alternatively we could support this in a generic way, perhaps providing an attribute to a wrapper on what parent element it needs to look for for the JS logic.

In this case, would it be okay to replace the getInPureModal with a flag passed from PHP to JS interface that would enable the same functionality (when the input id is not prepended by the form id)? Or do you prefer a more general approach where we allow to completely override the input ids' prefix?

michalkleiner commented 1 year ago

Thanks for the feedback. I understand your position and the end goal to making these two things work together. Let's see what @lekoala thinks about this and take it from there.

Alternatively we could support this in a generic way, perhaps providing an attribute to a wrapper on what parent element it needs to look for for the JS logic.

In this case, would it be okay to replace the getInPureModal with a flag passed from PHP to JS interface that would enable the same functionality (when the input id is not prepended by the form id)? Or do you prefer a more general approach where we allow to completely override the input ids' prefix?

Yep, something along those lines. Call it an alternative container or something along those lines and provide a data attribute from PHP to JS on what it is and then use that in the JS logic. Wouldn't go as far as complete freeform on the IDs.

lekoala commented 1 year ago

@michalkleiner @gurucomkz yes, there should be no custom adjustments for my module. I think it's just it never caused any issue for me but the fix should be on my end https://github.com/lekoala/silverstripe-pure-modal/commit/21c0501d85ba9e95873c8550955f74f73a385f00 this should do it

michalkleiner commented 1 year ago

Thanks @lekoala! Let me know if that solves your problem @gurucomkz and then we could close this issue. Cheers!

gurucomkz commented 1 year ago

Thank you @lekoala , the issue has been resolved! Thank you @michalkleiner for your attention!