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.

Sign up for GitHub

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

Jump to bottom

Feature: Support Multi-Step Paged Forms #18

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Feature: Support Multi-Step Paged Forms #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 36 additions & 12 deletions ...mponents/formalicious/model/formalicious/snippets/formalicioussnippetrenderform.class.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public function run(array $properties = [])
$hooks[] = 'FormaliciousHookHandleForm';
}

if ($currentStep >= $totalSteps) {
if ($currentStep >= $totalSteps && isset($_POST[$parameters['submitVar']])) {
if (!in_array('FormaliciousHookRemoveForm', $hooks, true)) {
$hooks[] = 'FormaliciousHookRemoveForm';
}
Expand Down Expand Up @@ -198,8 +198,30 @@ public function run(array $properties = [])
} else {
$hooks[] = 'redirect';

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 Can you please format this multi line comment like this?

/**
 * Sentence one.
 * Sentence two.
 */

Set up redirect in a way that allows variable/dynamic destination, based on
value stored in the submit input/button. This way, form data on the step that is
being navigated away from will always be posted/saved. Navigation and pagination
should then be done via submit inputs/buttons instead of <a> tags.

A related change to FormIt's Request class is necessary for this additional functionality.
*/

# Default redirect destination
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 Can you make this a single line PHP comment like this?
/* Default redirect destination. */

$destination = $currentStep + 1;

# Check for alternate destination request
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 And like this:
/* Check for alternate destination request. */

if ($_POST) {
foreach ($_POST as $k => $v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 Could you make the variable names minimum of three characters long?

foreach ($_POST as $key => $value) {

if (strpos($k, $parameters['submitVar']) !== false) {
$destination = is_numeric($v) ? $v : $destination ;
break;
}
}
}

$parameters['redirectTo'] = $this->getStepUrl([
$this->getProperty('stepParam') => $currentStep + 1
$this->getProperty('stepParam') => (int)$destination
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 Please add a whitespace after (int):

So:
(int) $destination

], 'full');

if (empty($placeholders['submitTitle'])) {
Expand All @@ -226,21 +248,23 @@ public function run(array $properties = [])
$parameters['formaliciousTplNavigationItem'] = $this->getProperty('tplNavigationItem');
$parameters['formaliciousTplNavigationWrapper'] = $this->getProperty('tplNavigationWrapper');

if ($currentStep <= 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think we need to remove prevUrl if current step less or equal 1? There will be problem if somebody use this placeholder and it will disappear in new version.

Can you please describe this or return prevUrl for first step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be not accounting for a particular scenario, but why would you show a previous URL if you're on the first step?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you. But if we delete this, we should change major version number I think. Because this broke backward compatibility. But maybe I'm wrong. I will ask somebody

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let me know. FYI, I verified that this change we're debating does not affect the functionality I'm introducing, so it can be reverted back if needed. If I remember, I made that change because the original line in the code caught my eye and didn't make sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 First of all, thank you for putting in the work!

I verified that this change we're debating does not affect the functionality I'm introducing, so it can be reverted back if needed.
Can you revert this part please, so we don't do any breaking changes?

Also could you add a section in the readme describing what you added and how that can be implemented? This will make it easier for other users to find out that it is possible and how they can achieve this.

$placeholders['prevUrl'] = $this->getStepUrl();
} else {
if ($currentStep > 1) {
$placeholders['prevUrl'] = $this->getStepUrl([
$this->getProperty('stepParam') => $currentStep - 1
]);
$placeholders['submitValPrev'] = $currentStep - 1;
}

if ($totalSteps === 1) {
$placeholders['currentUrl'] = $this->getStepUrl();
} else {
$placeholders['currentUrl'] = $this->getStepUrl([
$this->getProperty('stepParam') => $currentStep
]);
}
$placeholders['submitValLast'] = $totalSteps;

# Suggest renaming this placeholder, as it makes its purpose more immediately clear
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 Can you format this like:
/* Suggest renaming this placeholder, as it makes its purpose more immediately clear. */

$placeholders['formAction'] = $totalSteps === 1
Copy link
Contributor

Choose a reason for hiding this comment

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

@smg6511 Can you move the ; sign?

$placeholders['formAction'] = $totalSteps === 1
                    ? $this->getStepUrl()
                    : $this->getStepUrl([$this->getProperty('stepParam') => $currentStep]);

? $this->getStepUrl()
: $this->getStepUrl([$this->getProperty('stepParam') => $currentStep])
;

# Set orignal for backward compatibility
$placeholders['currentUrl'] = $placeholders['formAction'];

return $this->getChunk($this->getProperty('tplForm'), array_merge($placeholders, $parameters, [
'FormItParameters' => $this->parseParameters($parameters),
Expand Down