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

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

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions core/src/components/action-sheet/action-sheet.tsx
Expand Up @@ -255,12 +255,9 @@ export class ActionSheet implements ComponentInterface, OverlayInterface {

private async buttonClick(button: ActionSheetButton) {
const role = button.role;
if (isCancel(role)) {
return this.dismiss(button.data, role);
}
const shouldDismiss = await this.callButtonHandler(button);
if (shouldDismiss) {
return this.dismiss(button.data, button.role);
if (isCancel(role) || shouldDismiss) {
return this.dismiss(button.data, role);
}
return Promise.resolve();
}
Expand Down Expand Up @@ -289,9 +286,12 @@ export class ActionSheet 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.getButtons().find((b) => b.role === 'cancel');
const cancelButton = this.getButtons().find((b) => {
return b === button && b.role === 'cancel';
});
this.callButtonHandler(cancelButton);
}
};
Expand Down
14 changes: 7 additions & 7 deletions core/src/components/alert/alert.tsx
Expand Up @@ -466,12 +466,9 @@ export class Alert implements ComponentInterface, OverlayInterface {
private async buttonClick(button: AlertButton) {
const role = button.role;
const values = this.getValues();
if (isCancel(role)) {
return this.dismiss({ values }, role);
}
const returnData = await this.callButtonHandler(button, values);
if (returnData !== false) {
return this.dismiss({ values, ...returnData }, button.role);
if (isCancel(role) || returnData !== false) {
return this.dismiss({ values, ...returnData }, role);
}
return false;
}
Expand Down Expand Up @@ -673,9 +670,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
44 changes: 44 additions & 0 deletions core/src/components/alert/test/basic/alert.e2e.ts
Expand Up @@ -135,6 +135,50 @@ configs({ themes: ['light', 'dark'] }).forEach(({ config, screenshot, title }) =
});
});

configs().forEach(({ config, screenshot, title }) => {
test.describe(title('Test cancel buttons'), () => {
let alertFixture!: AlertFixture;

test.beforeEach(async ({ page }) => {
await page.goto('/src/components/alert/test/basic', config);
alertFixture = new AlertFixture(page, screenshot);
});

test('cancel button 1', async ({ page }) => {
await alertFixture.open('#multipleCancelButtons');
const optBtn1 = page.locator('ion-alert button.cancel1-btn');
await optBtn1.click();
const confirmOptBtn1Alert = page.locator('ion-alert[data-testid=cancel1-btn-clicked]');
const optBtn1AlertInfo = confirmOptBtn1Alert.locator('.alert-message').innerText();
const optBtn1AlertOkBtn = confirmOptBtn1Alert.locator('.alert-button-group button');
expect(await optBtn1AlertInfo).toBe('cancel1-btn-clicked');
await alertFixture.screenshot('alertCancelBtn1');
await optBtn1AlertOkBtn.click();
});

test('cancel button 2', async ({ page }) => {
await alertFixture.open('#multipleCancelButtons');
const optBtn2 = page.locator('ion-alert button.cancel2-btn');
await optBtn2.click();
const confirmOptBtn2Alert = page.locator('ion-alert[data-testid=cancel2-btn-clicked]');
const optBtn2AlertInfo = confirmOptBtn2Alert.locator('.alert-message').innerText();
const optBtn2AlertOkBtn = confirmOptBtn2Alert.locator('.alert-button-group button');
expect(await optBtn2AlertInfo).toBe('cancel2-btn-clicked');
await alertFixture.screenshot('alertCancelBtn2');
await optBtn2AlertOkBtn.click();
});

test('cancel button 3', async ({ page }) => {
await alertFixture.open('#multipleCancelButtons');
const ionAlertDidDismiss = await page.spyOnEvent('ionAlertDidDismiss');
const optBtn3 = page.locator('ion-alert button.cancel3-btn');
await optBtn3.click();
await ionAlertDidDismiss.next();
expect(ionAlertDidDismiss).toHaveReceivedEventDetail({ data: { values: undefined }, role: 'cancel' });
});
});
});

class AlertFixture {
readonly page: E2EPage;
readonly screenshotFn?: (file: string) => string;
Expand Down
47 changes: 47 additions & 0 deletions core/src/components/alert/test/basic/index.html
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,50 @@
});
}

function confirmAlertCancelBtns(expectedDataTestId) {
openAlert({
header: 'Alert',
subHeader: 'CancelBtnClicked',
message: expectedDataTestId,
buttons: ['OK'],
htmlAttributes: {
'data-testid': expectedDataTestId,
},
});
}

function presentAlertMultipleCancelButtons() {
openAlert({
header: 'Alert',
subHeader: 'Subtitle',
message: 'This is an alert message.',
buttons: [
{
text: 'Cancel 1',
role: 'cancel',
cssClass: 'cancel1-btn',
handler: () => {
confirmAlertCancelBtns('cancel1-btn-clicked');
},
},
{
text: 'Cancel 2',
role: 'cancel',
cssClass: 'cancel2-btn',
handler: () => {
confirmAlertCancelBtns('cancel2-btn-clicked');
},
},
{
text: 'Cancel 3',
role: 'cancel',
cssClass: 'cancel3-btn',
},
'Cancel',
],
});
}

function presentAlertNoMessage() {
openAlert({
header: 'Alert',
Expand Down
14 changes: 7 additions & 7 deletions core/src/components/picker/picker.tsx
Expand Up @@ -299,12 +299,9 @@ export class Picker implements ComponentInterface, OverlayInterface {

private async buttonClick(button: PickerButton) {
const role = button.role;
if (isCancel(role)) {
return this.dismiss(undefined, role);
}
const shouldDismiss = await this.callButtonHandler(button);
if (shouldDismiss) {
return this.dismiss(this.getSelected(), button.role);
if (isCancel(role) || shouldDismiss) {
return this.dismiss(this.getSelected(), role);
}
return Promise.resolve();
}
Expand Down Expand Up @@ -340,9 +337,12 @@ export class Picker 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.buttons.find((b) => b.role === 'cancel');
const cancelButton = this.buttons.find((b) => {
return b === button && b.role === 'cancel';
});
this.callButtonHandler(cancelButton);
}
};
Expand Down
51 changes: 51 additions & 0 deletions core/src/components/picker/test/basic/picker.e2e.ts
Expand Up @@ -26,3 +26,54 @@ configs().forEach(({ title, screenshot, config }) => {
});
});
});

configs({ modes: ['md'], directions: ['ltr', 'rtl'] }).forEach(({ title, config }) => {
test.describe(title('picker: button handlers'), () => {
test('should call cancel button handler', async ({ page }) => {
await page.setContent(
`
<script type="module">
import { pickerController } from '../../../../dist/ionic/index.esm.js';
window.pickerController = pickerController;
</script>

<button id="open" onclick="openPicker()">Open Picker</button>

<div id="result"></div>

<script>
async function openPicker() {
const picker = await pickerController.create({
buttons: [
{
text: 'Cancel',
role: 'cancel',
cssClass: 'cancel-btn',
handler: () => {
document.querySelector('#result').textContent = 'cancelled';
}
}
]
});
await picker.present();
}
</script>
`,
config
);

const didPresent = await page.spyOnEvent('ionPickerDidPresent');

const openBtn = page.locator('#open');
await openBtn.click();

await didPresent.next();

const cancelBtn = page.locator('ion-picker .cancel-btn');
await cancelBtn.click();

const result = page.locator('#result');
await expect(result).toHaveText('cancelled');
});
});
});
57 changes: 56 additions & 1 deletion core/src/components/toast/test/buttons/index.html
Expand Up @@ -14,7 +14,8 @@
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script>
</head>
<script type="module">
import { toastController } from '../../../../dist/ionic/index.esm.js';
import { alertController, toastController } from '../../../../dist/ionic/index.esm.js';
window.alertController = alertController;
window.toastController = toastController;
</script>
<body>
Expand All @@ -35,6 +36,10 @@
Multiple Buttons
</ion-button>

<ion-button id="multipleCancelButtons" expand="block" onclick="presentToastWithMultipleCancelButtons()">
Multiple Cancel Buttons
</ion-button>

<ion-button id="longButton" expand="block" onclick="presentToastWithLongButton()">
Really Long Button
</ion-button>
Expand All @@ -45,10 +50,16 @@
window.addEventListener('ionToastDidDismiss', function (e) {
console.log('didDismiss', e);
});

window.addEventListener('ionToastWillDismiss', function (e) {
console.log('willDismiss', e);
});

async function openAlert(opts) {
const alert = await alertController.create(opts);
await alert.present();
}

async function presentCloseArrayToast() {
const opts = {
message: 'Are you sure you want to delete this?',
Expand Down Expand Up @@ -118,6 +129,50 @@
return await presentToastWithOptions(opts);
}

function confirmAlertCancelBtns(expectedDataTestId) {
openAlert({
header: 'Alert',
subHeader: 'CancelBtnClicked',
message: expectedDataTestId,
buttons: ['OK'],
htmlAttributes: {
'data-testid': expectedDataTestId,
},
});
}

async function presentToastWithMultipleCancelButtons() {
const opts = {
message: 'Warning.',
buttons: [
{
text: 'C1',
role: 'cancel',
cssClass: 'cancel1-btn',
handler: () => {
confirmAlertCancelBtns('cancel1-btn-clicked');
},
},
{
text: 'C2',
role: 'cancel',
cssClass: 'cancel2-btn',
handler: () => {
confirmAlertCancelBtns('cancel2-btn-clicked');
},
},
{
text: 'C3',
role: 'cancel',
cssClass: 'cancel3-btn',
},
'Cancel',
],
};

return await presentToastWithOptions(opts);
}

async function presentToastWithLongButton() {
const opts = {
message: 'This item already has the label "travel". You can add a new label.',
Expand Down