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.

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

Add a confirmation message to forms and provide Ajax out of the box #5307

Merged
merged 31 commits into from
Jan 4, 2023

Conversation

qzminski
Copy link
Member

@qzminski qzminski commented Sep 23, 2022

I have added an option to define a confirmation message in the form settings. There is no longer a need to create a dedicated confirmation page – the confirmation will appear on the same page after a reload.

The redirect feature is still in place, however. Moreover, I have added the {{form_message}} insert tag, which can be used on the target page to display the confirmation message defined in the form settings.

Another significant change is that the form now works with ajax out of the box. This concept was taken from the well-known extension https://github.com/terminal42/contao-ajaxform

Problems to discuss:

  1. The JavaScript file is included without any URL fingerprint, which can cause cache issues. Perhaps we should switch to Webpack Encore to generate the assets?
  2. I am not entirely sure if the insert tags should be replaced using the replace() or replaceInline() method. @ausi can you shed some light on this?
  3. Shall we use Twig templates here already?

I'm looking forward to your feedback!

@Toflar Toflar added the feature label Sep 23, 2022
@Toflar Toflar added this to the 5.1 milestone Sep 23, 2022
@Toflar
Copy link
Member

Toflar commented Sep 23, 2022

Concept by yours truly 😇 I think it would be a great addition to the core, thanks for the PR @qzminski!

@ausi
Copy link
Member

ausi commented Sep 23, 2022

I am not entirely sure if the insert tags should be replaced using the replace() or replaceInline() method. @ausi can you shed some light on this?

Using replace() could result in having <esi> tags in the output, replaceInline() ensures that everything is replaced completely without <esi> tags. So replaceInline() is correct here I think.

Co-authored-by: Martin Auswöger <martin@auswoeger.com>
ausi
ausi previously approved these changes Sep 26, 2022
core-bundle/public/ajax-form.js Outdated Show resolved Hide resolved
core-bundle/public/ajax-form.js Outdated Show resolved Hide resolved
core-bundle/public/ajax-form.js Outdated Show resolved Hide resolved
core-bundle/public/ajax-form.js Outdated Show resolved Hide resolved
core-bundle/public/ajax-form.js Outdated Show resolved Hide resolved
core-bundle/public/ajax-form.js Outdated Show resolved Hide resolved
@qzminski
Copy link
Member Author

@ausi I have added your suggestions in one commit: 82d802c

Copy link
Member

@ausi ausi left a comment

Choose a reason for hiding this comment

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

#5307 (comment) needs to be addressed, rest approved ✅

Toflar
Toflar previously approved these changes Sep 28, 2022
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

Now that #5406 has been merged, this PR needs to be adjusted accordingly, doesn‘t it? @aschempp /cc

@qzminski
Copy link
Member Author

I have just pushed the commit that uses Webpack to generate the JS assets. Please review and confirm.

Toflar
Toflar previously approved these changes Dec 28, 2022
@Toflar Toflar requested review from m-vo, ausi, fritzmg and leofeyer December 28, 2022 12:13
ausi
ausi previously approved these changes Dec 28, 2022
@qzminski qzminski dismissed stale reviews from ausi and Toflar via b041914 January 3, 2023 16:11
@leofeyer leofeyer changed the title Add confirmation message to forms and provide ajax out of the box Add a confirmation message to forms and provide Ajax out of the box Jan 4, 2023
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

The changes look all good, but the problem remains that as soon as there is a confirmation message, the form is never rendered in the front end (see #5307 (review)).

@fritzmg
Copy link
Contributor

fritzmg commented Jan 4, 2023

I can confirm the issue. The problem is that the DCA field for the confirmation message and the template variable have the same name. The template will automatically be filled with all the tl_form data here:

$this->Template->setData($this->arrData);

And thus

<?php if ($this->confirmation): ?>

will always be true once a confirmation message has been set.

@leofeyer
Copy link
Member

leofeyer commented Jan 4, 2023

I have fixed the issue in 176c393.

@leofeyer leofeyer merged commit a9d7ca1 into contao:5.x Jan 4, 2023
@leofeyer
Copy link
Member

leofeyer commented Jan 4, 2023

Thank you @qzminski.

@qzminski qzminski deleted the feature/ajax-form branch January 13, 2023 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants