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 confirm message for form buttons. #11483

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
4 participants
@dereuromark
Copy link
Member

dereuromark commented Nov 30, 2017

Inside normal HTML using postLink() and confirm messages work fine.
But sometimes, working with a small actual form, you also would like to easily have confirm messages to confirm the form execution with "Are you sure..."?

<?= $this->Form->button(__('Finalize'), ['confirm' => 'Make sure ... before you continue.'] ?>
<?= $this->Form->end() ?>

Also works for postButton() then, of course, as standalone forms:

<?= $this->Form->postButton('Click', ['action' => 'foo', 'bar'], ['confirm' => 'Yeah?']) ?>

Not sure about the "old" Form->input() though..

@dereuromark dereuromark added this to the 3.5.7 milestone Nov 30, 2017

@ADmad

This comment has been minimized.

Copy link
Member

ADmad commented Nov 30, 2017

Not really thrilled about adding more support for inline JS :(

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 30, 2017

Codecov Report

Merging #11483 into master will increase coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11483      +/-   ##
============================================
+ Coverage      93.1%   93.38%   +0.28%     
- Complexity    13014    13015       +1     
============================================
  Files           436      436              
  Lines         33761    32751    -1010     
============================================
- Hits          31433    30586     -847     
+ Misses         2328     2165     -163
Impacted Files Coverage Δ Complexity Δ
src/View/Helper/FormHelper.php 96.22% <100%> (-0.02%) 369 <0> (+1)
src/Console/ConsoleInput.php 21.42% <0%> (-5.24%) 6% <0%> (ø)
src/Network/Exception/GoneException.php 75% <0%> (-5%) 2% <0%> (ø)
.../Network/Exception/ServiceUnavailableException.php 75% <0%> (-5%) 2% <0%> (ø)
src/Network/Exception/InternalErrorException.php 75% <0%> (-5%) 2% <0%> (ø)
...rc/Network/Exception/InvalidCsrfTokenException.php 75% <0%> (-5%) 2% <0%> (ø)
src/Network/Exception/ForbiddenException.php 75% <0%> (-5%) 2% <0%> (ø)
...Routing/Exception/DuplicateNamedRouteException.php 75% <0%> (-5%) 3% <0%> (ø)
.../Exception/UnavailableForLegalReasonsException.php 75% <0%> (-5%) 2% <0%> (ø)
src/Network/Exception/NotAcceptableException.php 75% <0%> (-5%) 2% <0%> (ø)
... and 307 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbd230b...9b72115. Read the comment docs.

@dereuromark

This comment has been minimized.

Copy link
Member

dereuromark commented Nov 30, 2017

@ADmad As long as we do not have a good concept to move away from the already existing functionality, completing it is a must-have IMO :)

Once there is a concept for it, we can easily then migrate all of them together with a future major version jump.
You are also free to not use this functionality, but people needing the confirmation tooling sure would love to have it working at ease together with the existing ones. So completely opt-in and BC.

@markstory

This comment has been minimized.

Copy link
Member

markstory commented Nov 30, 2017

I share @ADmad's thoughts on inline javascript, but also recognize we don't have a better solution right now. Hopefully in a future release we can replace the inline JS with event bindings.

@markstory markstory self-assigned this Nov 30, 2017

@markstory markstory merged commit d7cf3e7 into master Dec 1, 2017

6 checks passed

codecov/patch 100% of diff hit (target 93.1%)
Details
codecov/project 93.38% (+0.28%) compared to fbd230b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.

@markstory markstory deleted the master-button-confirm branch Dec 1, 2017

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