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.

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

[fix](alert, actionSheet, picker and toast): Call pressed button handler, regardless 'role' #28952

Closed
wants to merge 25 commits into from
+314 −28
Closed

[fix](alert, actionSheet, picker and toast): Call pressed button handler, regardless 'role' #28952

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
5bfcd13
[fix](alert): call pressed button handler, regardless 'role'
joselrio Jan 29, 2024
4ce2e61
[fix](alert): rollback unintended removed line.
joselrio Jan 29, 2024
708ad15
[fix](alert): update to improve code readability.
joselrio Jan 31, 2024
e47e139
Merge branch 'main' into alert-cancel-buttons-update
joselrio Feb 1, 2024
07236f1
[fix](alert): Run lint.fix after PR test-core-lint fail.
joselrio Feb 1, 2024
570270b
[fix](alert): Adding validation if button handler returns false.
joselrio Feb 9, 2024
2be3bd9
[fix](action-sheet): call pressed button handler, regardless 'role'
joselrio Feb 9, 2024
42e0c83
[fix](picker): call pressed button handler, regardless 'role'
joselrio Feb 9, 2024
14ba21b
[fix](toast): call pressed button handler, regardless 'role'
joselrio Feb 9, 2024
958a4fa
Merge branch 'main' into alert-cancel-buttons-update
joselrio Feb 9, 2024
69bf9a2
[fix](alert): Tests to ensure proper handler is triggered
joselrio Mar 12, 2024
23531b4
Merge branch 'main' into alert-cancel-buttons-update
joselrio Mar 12, 2024
693a2c3
[fix](alert): lint.fix
joselrio Mar 12, 2024
2d9761c
[fix](alert): Method renamed.
joselrio Mar 12, 2024
610e275
[fix](picker): Tests to ensure proper handler is triggered.
joselrio Mar 12, 2024
6135478
[fix](alert): lint
joselrio Mar 12, 2024
0edd1a0
[fix](alert): Split cancel buttons tests.
joselrio Mar 12, 2024
6967dda
[fix](picker): Split cancel buttons tests.
joselrio Mar 12, 2024
621d6b2
[fix](toast): Tests to ensure proper handler is triggered.
joselrio Mar 12, 2024
f01cabe
[fix](toast): Removed unused property.
joselrio Mar 13, 2024
b0910ed
[fix](toast): Fixed test issues and add screenshots.
joselrio Mar 13, 2024
d05e653
[fix](alert): Added screenshot test.
joselrio Mar 13, 2024
6ce83c4
[fix](picker): Added screenshot test.
joselrio Mar 13, 2024
bbd9cda
[fix](toast): Fixed screenshot and test names.
joselrio Mar 13, 2024
91b5b8b
test(picker): cancel button handler is called
sean-perkins Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions core/src/components/alert/alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,12 +462,12 @@ export class Alert implements ComponentInterface, OverlayInterface {
private async buttonClick(button: AlertButton) {
const role = button.role;
const values = this.getValues();
if (isCancel(role)) {
if (isCancel(role) && button.handler === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll need to ask the team, but this change does change the current behavior of a cancel role button with a custom handler that returns false.

Previously, it would have dismissed the overlay. With the proposed changes it would not dismiss.

I think the change in behavior is valid/expected, but never hurts to get a second opinion 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello! After speaking with the team, I'd advise we adjust the implementation pattern here. We should de-risk and validate a use case for needing to prevent a dismiss with a cancel role, before making that change.

The proposed fix for the original issue should execute the handler callback, but not prevent dismissing. Something to the effect of:

private async buttonClick(button: AlertButton) {
  const role = button.role;
  const values = this.getValues();
  const returnData = await this.callButtonHandler(button, values);
  if (isCancel(role)) {
    /**
     * Cancel roles should always dismiss the alert,
     * even if the handler function returns `false`.
     */
    return this.dismiss({ values, ...returnData }, role);
  }
  if (returnData !== false) {
    return this.dismiss({ values, ...returnData }, role);
  }
  return false;
}

While investigating this issue further, I found that ion-action-sheet, ion-picker, and ion-toast all have very similar implementations and likely the same problematic behavior.

We can discuss more in Slack, but I think addressing the fix across all these implementations would be great to knock out together, once we validate the path for one. I can help out here if desired.

return this.dismiss({ values }, role);
}
const returnData = await this.callButtonHandler(button, values);
if (returnData !== false) {
return this.dismiss({ values, ...returnData }, button.role);
return this.dismiss({ values, ...returnData }, role);
}
return false;
}
Expand Down Expand Up @@ -669,9 +669,12 @@ export class Alert implements ComponentInterface, OverlayInterface {
};

private dispatchCancelHandler = (ev: CustomEvent) => {
const role = ev.detail.role;
const button = ev.detail;
const role = button.role;
if (isCancel(role)) {
const cancelButton = this.processedButtons.find((b) => b.role === 'cancel');
const cancelButton = this.processedButtons.find((b) => {
return b === button && b.role === 'cancel';
});
this.callButtonHandler(cancelButton);
}
};
Expand Down
32 changes: 32 additions & 0 deletions core/src/components/alert/test/basic/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
<button class="custom-accordion-button" id="basic" onclick="presentAlert()">Alert</button>
<button id="longMessage" onclick="presentAlertLongMessage()">Alert Long Message</button>
<button id="multipleButtons" onclick="presentAlertMultipleButtons()">Multiple Buttons (>2)</button>
<button id="multipleCancelButtons" onclick="presentAlertMultipleCancelButtons()">
Multiple Cancel Buttons (>2)
</button>
<button id="noMessage" onclick="presentAlertNoMessage()">Alert No Message</button>
<button id="asyncHandler" onclick="presentAlertAsyncHandler()">Alert Async Handler</button>
<button id="confirm" onclick="presentAlertConfirm()">Confirm</button>
Expand Down Expand Up @@ -115,6 +118,35 @@
});
}

function presentAlertMultipleCancelButtons() {
openAlert({
header: 'Alert',
subHeader: 'Subtitle',
message: 'This is an alert message.',
buttons: [
{
text: 'Cancel 1',
role: 'cancel',
handler: () => {
console.log('Cancel btn 1');
},
},
{
text: 'Cancel 2',
role: 'cancel',
handler: () => {
console.log('Cancel btn 2');
},
},
{
text: 'Cancel 3',
role: 'cancel',
},
'Cancel',
],
});
}

function presentAlertNoMessage() {
openAlert({
header: 'Alert',
Expand Down
Loading