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

Optionsets no longer work #69

Closed UndefinedOffset closed 1 year ago

UndefinedOffset commented 8 years ago

Looks like something in #54 has broken Optionsets, they can't be a master and can't be used as part of logic. I haven't yet been able to isolate the issue specifically but the simple solution could be to just a modified OptionsetField_Holder.ss to the templates/forms folder that uses HolderID instead of $Name on the outer div.

UndefinedOffset commented 8 years ago

I should note that the field in question's name would actually end up being SomeWrapper[FieldName]. Some change in the pull in question it causes the name looked up to be SomeWrapper_FieldName which does not match to the field using the current optionset hacks.

jacobsjensen commented 8 years ago

Looks like CheckboxSetField doesn't work either...

unclecheese commented 8 years ago

OptionsetField and its related CheckboxSetField have been a nuisance to this module since day one, due to their idiosyncratic structure. Further, the form templates seem to be a bit of a moving target with each release of the framework, and I just have a bad feeling that we're on the wrong path.

@UndefinedOffset / @jacobsjensen I'd be very keen to get your feedback on a new API proposal I scratched out a few weeks ago here (https://github.com/unclecheese/silverstripe-display-logic/issues/67#issuecomment-244820215)

tl;dr DisplayLogicWrapper all the things. What do you reckon?

UndefinedOffset commented 8 years ago

I had a look at that change, I think it makes allot of sense to do that. It does introduce some redundancy/inconvenience but at the same time it fixes so many other issues that it makes allot of sense. That said it's a pretty major/breaking change it may make sense to have a period where the old stuff is just deprecated and throws warnings accordingly.

patricknelson commented 7 years ago

This could just be considered a known/accepted limitation (due to the maintenance involved that @unclecheese outlined). So, should we update the README.md to incorporate a caveat? e.g.

$fieldWrapper = new DisplayLogicWrapper(new OptionsetField('SomeID', 'Some Field', $fieldOptions);
$fieldWrapper->addExtraClass('field'); // Necessary for design consistency most of the time...
$fieldWrapper->displayIf('AnotherField')->isChecked();

I've found myself organically re-discovering this limitation and happening upon this exact github issue.

michalkleiner commented 1 year ago

Closing this issue since it's outdated and we're now on Silverstripe CMS 4.12.0 and Display Logic 2.0.5.

If the problem persists feel free to open a new bug report and provide replication steps and information on what version of Silverstripe CMS and the module you're using.