Skip to content

Conversation

@mjauvin
Copy link
Member

@mjauvin mjauvin commented May 15, 2023

There is currently no way for a partial to know the form's previewMode, especially when in a repeater or nestedform firmwidget.

@mjauvin mjauvin added enhancement PRs that implement a new feature or substantial change needs review Issues/PRs that require a review from a maintainer labels May 15, 2023
@mjauvin mjauvin added this to the v1.2.3 milestone May 15, 2023
@mjauvin mjauvin requested a review from bennothommo May 15, 2023 15:49
@mjauvin mjauvin self-assigned this May 15, 2023
@bennothommo

This comment was marked as outdated.

@LukeTowers
Copy link
Member

Why does it need to?

@AIC-BV
Copy link
Contributor

AIC-BV commented May 16, 2023

Why does it need to?

Because when you are in update context and your repeater is disabled,
All fields in the repeater are disabled except the partials (they don't get saved but they are visually enabled)
image

_mypartial.htm
<?php if ($this->previewMode): ?>
    <span class="form-control"><?= isset($field->value) ? e($field->value) : '&nbsp;' ?></span>
<?php else: ?>
    ... This code is executed
<?php endif ?>

@bennothommo
Copy link
Member

@LukeTowers I spoke with @AIC-BV on Discord and the issue is that previewMode is set to true for a repeater or nested form if they are disabled (regardless of the context), so it's expected that all fields within should be shown in a preview context in that case, but with partials there doesn't seem to be a way to get that previewMode variable.

Copy link
Member

@bennothommo bennothommo left a comment

Choose a reason for hiding this comment

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

All good to be merged in with v1.2.3

@AIC-BV
Copy link
Contributor

AIC-BV commented May 16, 2023

It looks fine, but it is not working 😅
Its true in _field_partial.php, but still null in my partial

@bennothommo
Copy link
Member

bennothommo commented May 16, 2023

@AIC-BV are you using $this->previewMode or $previewMode in your partial? Because you should be using the second one ;)

@AIC-BV
Copy link
Contributor

AIC-BV commented May 16, 2023

@AIC-BV are you using $this->previewMode or $previewMode in your partial? Because you should be using the second one ;)

Yes 🤦‍♂️
<?php if ($this->previewMode || $previewMode): ?>
That works! 😁

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2023

Can we merge this one with 1.2.2 as well @LukeTowers it's definitely not going to break anything, it just add a variable for the partial.

@LukeTowers
Copy link
Member

Not yet @mjauvin, I'm still not convinced of it and it adds to the API surface. Why is previewMode necessary and not just looking at the disabled state of the current field?

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2023

Not yet @mjauvin, I'm still not convinced of it and it adds to the API surface. Why is previewMode necessary and not just looking at the disabled state of the current field?

That information is NOT available elsewhere when within a repeater/nestedform @LukeTowers

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2023

@LukeTowers the only information sent to the partial is this:

<?= $this->controller->makePartial($field->path ?: $field->fieldName, [
    'formModel' => $formModel,
    'formField' => $field,
    'formValue' => $field->value,
    'model'     => $formModel,
    'field'     => $field,
    'value'     => $field->value
]) ?>       

model is not useful for previewMode
field does not contain this information (unlike other formwidgets).

Now it would be possible to set the previewMode in the field as well (from backend/widgets/Form.php) but I find it more intrusive.

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2023

@LukeTowers I can also do the following if you prefer:

diff --git a/modules/backend/widgets/form/partials/_field_partial.php b/modules/backend/widgets/form/partials/_field_partial.php
index ae545876a..5b6f54277 100644
--- a/modules/backend/widgets/form/partials/_field_partial.php
+++ b/modules/backend/widgets/form/partials/_field_partial.php
@@ -1,3 +1,5 @@
+<?php $field->previewMode = $this->previewMode ?>
+
 <?= $this->controller->makePartial($field->path ?: $field->fieldName, [
     'formModel' => $formModel,
     'formField' => $field,
@@ -5,5 +7,4 @@
     'model'     => $formModel,
     'field'     => $field,
     'value'     => $field->value,
-    'previewMode' => $this->previewMode,
 ]) ?>

@mjauvin
Copy link
Member Author

mjauvin commented May 17, 2023

That works too, but how is it different about the API surface ?

@LukeTowers
Copy link
Member

Because it's more purposeful. Adding the variable previewMode to partials is non-standard and solves only this issue, adding the variable formWidget follows the convention of the other variables that are already provided, solves the current issue, and makes the partial field have a more similar set of variables to what is available to other field type partials; therefore hopefully solving other potential future issues.

@LukeTowers LukeTowers added Status: Completed and removed needs review Issues/PRs that require a review from a maintainer labels May 17, 2023
@LukeTowers LukeTowers modified the milestones: v1.2.3, v1.2.2 May 17, 2023
@LukeTowers LukeTowers changed the title Make Form previewMode available in partial field variables Provide the current Form instance to partials May 17, 2023
@LukeTowers LukeTowers merged commit 3341a47 into develop May 17, 2023
@LukeTowers LukeTowers deleted the partial-preview-mode branch May 17, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement PRs that implement a new feature or substantial change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants