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

Display the form success and error message at the bottom #10726

Merged
merged 11 commits into from May 3, 2023

Conversation

volha-pivavarchyk
Copy link
Contributor

@volha-pivavarchyk volha-pivavarchyk commented Jan 4, 2022

Q A
Bug fix? (use the a.b branch) [x]
New feature/enhancement? (use the a.x branch) [ ]
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? [ ]
Issue(s) addressed Fixes #7279

Description:

This PR adds a global setting to select the location of the form success and error messages.

image

Steps to test this PR:

  1. Create a new form and select 'display message' as Successful Submit Action
  2. Add the form to the landing page
  3. Open the page and submit the form
  4. The message is at the top of the form
  5. Change the Successful Submit Action setting to at the bottom
  6. Repeat the steps 1-3
  7. The message is at the bottom of the form

@cla-bot
Copy link

cla-bot bot commented Jan 4, 2022

Thank you for your contribution! We require all contributors to sign our Contributor License Agreement, and we do not have a record of your signature on file. In order for us to review and merge your code, please head over to https://www.mautic.org/contributor-agreement and complete the form. There may be a short delay while the team add you as a contributor - please be patient :). Any problems contact the Product Team on Slack (get an invite at https://mautic.org/slack). CLA has not been signed by @volha-pivavarchyk.

@escopecz
Copy link
Sponsor Member

escopecz commented Jan 5, 2022

@volha-pivavarchyk what if some users want to have the form at the top of the form?

I'd suggest to make this change in the CSS file of the website where you embed the form. Would that work for you?

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 5, 2022
@adiux
Copy link
Contributor

adiux commented Jan 6, 2022

Hi @escopecz, thanks for reviewing this.

IMO it is not a good practice to show the error message on top. With longer forms, the user will not see the error or success message. This could lead to a bad user experience, or maybe even people aborting the form. Especially on mobile, this will be a problem.

For these reasons, I think we should make it the default to display the messages at the bottom.

@RCheesley
Copy link
Sponsor Member

@cla-bot check

@cla-bot
Copy link

cla-bot bot commented Jan 6, 2022

The CLA Bot has been sent on a mission to check against the latest list and will be back shortly with its findings!

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Jan 6, 2022
kuzmany
kuzmany previously approved these changes Jan 6, 2022
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

I am okay with the change, on large forms is better.
But I do not know how to test error message

👍

@kuzmany
Copy link
Member

kuzmany commented Jan 6, 2022

But it's BC. The best would add option to display success message on the top/bottom (but that's unnecessarily much)

@RCheesley
Copy link
Sponsor Member

I am inclined to agree with @adiux - when you fill in a form, if it has more than one or two fields you end up scrolled down the page when you submit, and never see the message unless you happen to scroll back up. I think it makes more sense, and in more aligned with best practices, to be showing where the user is at the time.

We definitely had this discussion before but I am struggling to find where - I have found an issue which I linked to the PR though.

Generally speaking it is best practice from what I have been reading to show the message as close to where the action / error has taken place as possible, and always in the line of sight of the user when the action took place.

Some useful research here although it relates to error messages rather than confirmations, but I feel that the general gist is the same.

@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #10726 (2ed1643) into 5.x (2046813) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##                5.x   #10726   +/-   ##
=========================================
  Coverage     55.79%   55.79%           
  Complexity    35898    35898           
=========================================
  Files          2256     2256           
  Lines        107588   107590    +2     
=========================================
+ Hits          60026    60028    +2     
  Misses        47562    47562           
Impacted Files Coverage Δ
...pp/bundles/FormBundle/Form/Type/ConfigFormType.php 100.00% <100.00%> (ø)
app/bundles/FormBundle/Model/FormModel.php 77.44% <100.00%> (+0.05%) ⬆️

@escopecz
Copy link
Sponsor Member

I agree with the best practices and we should have done that at the beginning, but it's too late now.

The problem I have with this change are:

  1. It's a BC break. Users tend to style the forms on the website where the form is embedded into. This change make break that styling. So as Kuzmany suggests, this should be configurable and by default it should be at the top to keep the backward compatibility for existing forms. What if some users modified their forms the way that the browser scrolls up on submit?
  2. The change is done by a CSS hack rather than moving the HTML elements to the bottom of the form in the HTML form itself https://github.com/mautic/mautic/blob/4.x/app/bundles/FormBundle/Views/Builder/form.html.php#L58-L59.

@adiux
Copy link
Contributor

adiux commented Jan 11, 2022

@escopecz

  1. Since we are only changing the position of the HTML element, not the element itself or the ID, the JS scrolling should still work. It is just located somewhere else

  2. We can change it to HTML

@escopecz escopecz added forms Anything related to forms ready-to-test PR's that are ready to test user-experience Anything related to related to workflows, feedback, and navigation and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Jan 13, 2022
escopecz
escopecz previously approved these changes Jan 13, 2022
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

This is fine from the code perspective. I will let others decide on the potential BC break.

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged T1 Low difficulty to fix (issue) or test (PR) and removed ready-to-test PR's that are ready to test labels Jan 13, 2022
@kuzmany
Copy link
Member

kuzmany commented Jan 13, 2022

But agree with John. Customers customize forms by CSS/templates and this PR change their customization (even If like it). We cannot do that without customer confirmation

Copy link

@ZebruhDivs ZebruhDivs left a comment

Choose a reason for hiding this comment

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

I was getting an error with the GitPod, but based on the conversation, I agree with Ruth.
It's better to have in-line error messaging per field instead of having a long message either at the top or bottom. In-line errors provide context and prevent the user from having to search for the field in error.

The ideal combination would be having a general alert - either a toast, a flash alert, or an alert at the top of the form (making sure that the form starts at the top once the form is entered) with general message along the lines of "Oops! There was an error submitting the form" in combination with in-line errors. The general alert will let users know the form was not submitted and help with the viewport issue (in case there is an in-line error below the fold), and the in-line errors will have specific in-context information around what went wrong and how to fix it.

These general errors cannot be the only form of error notification, however. Providing a success version of this error would also be helpful (someone mentioned this as well).

I'm including an example of this combination from Nielsen Norman, along with some other error best practices.
survey-monkey-error

https://www.nngroup.com/articles/errors-forms-design-guidelines/

I'm a bit unclear about what ARIA labels are required for accessibility with this format.

Copy link

@ZebruhDivs ZebruhDivs left a comment

Choose a reason for hiding this comment

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

Another great option (even more ideal, if it's possible with the software), would be to disable the "submit" button until all the fields are filled in correctly, and have the in-line messaging appear as the user navigates out of each field.

@adiux
Copy link
Contributor

adiux commented Aug 18, 2022

I agree with your comments. They are all great additions. However, this was supposed to be a quick fix to improve the current behaviour. For me, they are not related to this PR. This PR is only about moving the message from the top of the form to the bottom. I think this will already be a good usability improvement. For the other changes, we should add a separate feature request.

So for this PR to move forward, we need to answer these two questions:

  • do we want to move the message down?
  • is it a BC break?

@kuzmany
Copy link
Member

kuzmany commented Aug 21, 2022

  • do we want to move the message down?

As option, yes. But I like it.

  • is it a BC break?

Yes, it is. Customers have design forms on theirs pages/popups etc. This change layout of form a lil bit

@adiux
Copy link
Contributor

adiux commented Oct 3, 2022

@kuzmany Would you be ok if we add a global setting for all forms (in the local.php)?

Since this is a BC break, it would be version 5?

@kuzmany
Copy link
Member

kuzmany commented Oct 4, 2022

@adiux yes, global parameter in local.php is ok for me 👍

@volha-pivavarchyk volha-pivavarchyk changed the base branch from 4.x to 5.x October 5, 2022 17:02
@adiux
Copy link
Contributor

adiux commented Oct 10, 2022

@volha-pivavarchyk can you please fix the conflicts?

@adiux
Copy link
Contributor

adiux commented Jan 26, 2023

@volha-pivavarchyk could you please resolve the conflicts again. @RCheesley would be great if we can get this merged

@mabumusa1 mabumusa1 added this to the 5.0-alpha milestone Mar 3, 2023
@mabumusa1 mabumusa1 added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Mar 3, 2023
@mabumusa1
Copy link
Member

@volha-pivavarchyk please fix the conflicts, I think we can merge this one today

# Conflicts:
#	app/bundles/FormBundle/Config/config.php
#	app/bundles/FormBundle/Form/Type/ConfigFormType.php
#	app/bundles/FormBundle/Model/FormModel.php
#	app/bundles/FormBundle/Tests/FormTestAbstract.php
#	app/bundles/FormBundle/Tests/Model/DeleteFormTest.php
#	app/bundles/FormBundle/Translations/en_US/messages.ini
#	app/bundles/FormBundle/Views/Builder/form.html.php
@volha-pivavarchyk
Copy link
Contributor Author

volha-pivavarchyk commented Mar 4, 2023

@mabumusa1 I resolved the conflicts and fixed issues

@kuzmany kuzmany removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Mar 6, 2023
ghost
ghost previously approved these changes Apr 28, 2023
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi, my test is successful.

Created form with two fields: email and text. Added as final action: Display Message and entered a Thankyou text.

With the option "at the top" (chosen by default):
image

With the option "at the bottom":
image

The Combobox is located in the Form Settings:
image

@escopecz
Copy link
Sponsor Member

escopecz commented May 3, 2023

@volha-pivavarchyk could you please once more resolve the conflicts?

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label May 3, 2023
# Conflicts:
#	app/bundles/FormBundle/Model/FormModel.php
@volha-pivavarchyk
Copy link
Contributor Author

@escopecz I've resolved

@escopecz escopecz removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label May 3, 2023
@escopecz escopecz merged commit 4d60ca0 into mautic:5.x May 3, 2023
11 checks passed
@ghost
Copy link

ghost commented May 5, 2023

Can the changes be available also in V4?

@escopecz
Copy link
Sponsor Member

escopecz commented May 9, 2023

@IonutOjica sadly not. This is an enhancement. Not a bug fix. M4 can get only bug fixes. Let's work all together to release M5 so we can get all these goodies with it.

@ghost ghost mentioned this pull request May 9, 2023
@escopecz escopecz added the enhancement Any improvement to an existing feature or functionality label Jun 23, 2023
@mautibot
Copy link

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/mautic-5-beyond-expectations-beyond-limits/30459/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The PR contributors have signed the contributors agreement enhancement Any improvement to an existing feature or functionality forms Anything related to forms pending-test-confirmation PR's that require one test before they can be merged T1 Low difficulty to fix (issue) or test (PR) user-experience Anything related to related to workflows, feedback, and navigation
Projects
None yet
8 participants