Skip to content

Feature: Support Multi-Step Paged Forms#18

Draft
smg6511 wants to merge 1 commit intoSterc:developfrom
smg6511:feature-multistep-post
Draft

Feature: Support Multi-Step Paged Forms#18
smg6511 wants to merge 1 commit intoSterc:developfrom
smg6511:feature-multistep-post

Conversation

@smg6511
Copy link
Contributor

@smg6511 smg6511 commented Dec 1, 2021

Update: Converted this to a draft, as it applies to an outdated implementation of Formalicious. See new port at PR #39.

Adds ability to have multiple submit buttons/inputs. With this change (and a specific method of setup in the form templates), form data can be posted when navigating away from the current form step.

Note: To use this feature, an addition/change to Formit is required as well (see PR 259 on Formit repo).

Adds ability to have multiple submit buttons/inputs. With this change (and a specific method of setup in the form templates), form data can be posted when navigating away from the current form step.
$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.

} 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.
 */

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. */

# 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. */


# 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) {


$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

}
$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['submitValLast'] = $totalSteps;

# 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]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants