Skip to content

Add confirm message for form buttons. #11483

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
Merged
markstory merged 1 commit into from
Dec 1, 2017
Merged

Add confirm message for form buttons. #11483

markstory merged 1 commit into from
Dec 1, 2017

Conversation

@dereuromark
Copy link
Member

@dereuromark dereuromark commented Nov 30, 2017 •
edited
Loading

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
Copy link
Member

ADmad commented Nov 30, 2017

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

@codecov-io
Copy link

codecov-io commented Nov 30, 2017 •
edited
Loading

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
Copy link
Member Author

dereuromark commented Nov 30, 2017 •
edited
Loading

@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
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
@markstory markstory deleted the master-button-confirm branch December 1, 2017 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@markstory markstory

Labels

enhancement

Projects

None yet

Milestone

3.5.7

Development

Successfully merging this pull request may close these issues.

5 participants

@dereuromark @ADmad @codecov-io @markstory