-
Notifications
You must be signed in to change notification settings - Fork 13.5k
feat(select): add close button text property #30282
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
Open
niconaso
wants to merge
5
commits into
ionic-team:main
Choose a base branch
from
niconaso:feature/add-close-text
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
7f23cc0
feat(select): allow to pass custom close button text
b5d3dc9
fix(select-modal): delete old screenshots
0c5497d
fix(select-modal): update screenshots
f38b7eb
fix(select-modal): rename closeText to cancelText
14c7d9c
Merge branch 'main' into feature/add-close-text
niconaso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
40 changes: 40 additions & 0 deletions
40
core/src/components/select-modal/test/custom-close-text/index.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<!DOCTYPE html> | ||
<html lang="en" dir="ltr"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<title>Select - Modal</title> | ||
<meta | ||
name="viewport" | ||
content="width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no" | ||
/> | ||
<link href="../../../../../css/ionic.bundle.css" rel="stylesheet" /> | ||
<link href="../../../../../scripts/testing/styles.css" rel="stylesheet" /> | ||
<script src="../../../../../scripts/testing/scripts.js"></script> | ||
<script nomodule src="../../../../../dist/ionic/ionic.js"></script> | ||
<script type="module" src="../../../../../dist/ionic/ionic.esm.js"></script> | ||
</head> | ||
|
||
<body> | ||
<ion-app> | ||
<ion-header> | ||
<ion-toolbar> | ||
<ion-title>Select Modal - Custom Close Text</ion-title> | ||
</ion-toolbar> | ||
</ion-header> | ||
|
||
<ion-content> | ||
<ion-modal is-open="true"> | ||
<ion-select-modal multiple="false" close-text="Close me"></ion-select-modal> | ||
</ion-modal> | ||
</ion-content> | ||
</ion-app> | ||
|
||
<script> | ||
const selectModal = document.querySelector('ion-select-modal'); | ||
selectModal.options = [ | ||
{ value: 'apple', text: 'Apple', disabled: false, checked: true }, | ||
{ value: 'banana', text: 'Banana', disabled: false, checked: false }, | ||
]; | ||
</script> | ||
</body> | ||
</html> |
101 changes: 101 additions & 0 deletions
101
core/src/components/select-modal/test/custom-close-text/select-modal.e2e.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
import { expect } from '@playwright/test'; | ||
import { configs, test } from '@utils/test/playwright'; | ||
|
||
import type { SelectModalOption } from '../../select-modal-interface'; | ||
import { SelectModalPage } from '../fixtures'; | ||
|
||
const options: SelectModalOption[] = [ | ||
{ value: 'apple', text: 'Apple', disabled: false, checked: false }, | ||
{ value: 'banana', text: 'Banana', disabled: false, checked: false }, | ||
]; | ||
|
||
const checkedOptions: SelectModalOption[] = [ | ||
{ value: 'apple', text: 'Apple', disabled: false, checked: true }, | ||
{ value: 'banana', text: 'Banana', disabled: false, checked: false }, | ||
]; | ||
|
||
/** | ||
* This behavior does not vary across modes/directions. | ||
*/ | ||
configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { | ||
test.describe(title('select-modal: custom-close-text'), () => { | ||
test.beforeEach(({ browserName }) => { | ||
test.skip(browserName === 'webkit', 'ROU-5437'); | ||
}); | ||
|
||
test.describe('single selection', () => { | ||
let selectModalPage: SelectModalPage; | ||
|
||
test.beforeEach(async ({ page }) => { | ||
selectModalPage = new SelectModalPage(page); | ||
}); | ||
|
||
test('clicking an unselected option should dismiss the modal', async () => { | ||
await selectModalPage.setup(config, options, false); | ||
|
||
await selectModalPage.clickOption('apple'); | ||
await selectModalPage.ionModalDidDismiss.next(); | ||
await expect(selectModalPage.modal).not.toBeVisible(); | ||
}); | ||
|
||
test('clicking a selected option should dismiss the modal', async () => { | ||
await selectModalPage.setup(config, checkedOptions, false); | ||
|
||
await selectModalPage.clickOption('apple'); | ||
await selectModalPage.ionModalDidDismiss.next(); | ||
await expect(selectModalPage.modal).not.toBeVisible(); | ||
}); | ||
|
||
test('pressing Space on an unselected option should dismiss the modal', async () => { | ||
await selectModalPage.setup(config, options, false); | ||
|
||
await selectModalPage.pressSpaceOnOption('apple'); | ||
await selectModalPage.ionModalDidDismiss.next(); | ||
await expect(selectModalPage.modal).not.toBeVisible(); | ||
}); | ||
|
||
test('pressing Space on a selected option should dismiss the modal', async ({ browserName }) => { | ||
test.skip(browserName === 'firefox', 'Same behavior as ROU-5437'); | ||
|
||
await selectModalPage.setup(config, checkedOptions, false); | ||
|
||
await selectModalPage.pressSpaceOnOption('apple'); | ||
await selectModalPage.ionModalDidDismiss.next(); | ||
await expect(selectModalPage.modal).not.toBeVisible(); | ||
}); | ||
|
||
test('clicking the close button should dismiss the modal', async () => { | ||
await selectModalPage.setup(config, options, false); | ||
|
||
const closeButton = selectModalPage.modal.locator('ion-header ion-toolbar ion-button'); | ||
await closeButton.click(); | ||
await selectModalPage.ionModalDidDismiss.next(); | ||
await expect(selectModalPage.modal).not.toBeVisible(); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
/** | ||
* This behavior does not vary across directions. | ||
* The components used inside of `ion-select-modal` | ||
* do have RTL logic, but those are tested in their | ||
* respective component test files. | ||
*/ | ||
configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => { | ||
test.describe(title('select-modal: rendering'), () => { | ||
let selectModalPage: SelectModalPage; | ||
|
||
test.beforeEach(async ({ page }) => { | ||
selectModalPage = new SelectModalPage(page); | ||
}); | ||
test('should not have visual regressions with single selection', async () => { | ||
await selectModalPage.setup(config, checkedOptions, false); | ||
await selectModalPage.screenshot(screenshot, 'select-modal-diff'); | ||
}); | ||
test('should not have visual regressions with multiple selection', async () => { | ||
await selectModalPage.setup(config, checkedOptions, true); | ||
await selectModalPage.screenshot(screenshot, 'select-modal-multiple-diff'); | ||
}); | ||
}); | ||
}); |
Binary file added
BIN
+6.34 KB
...elect-modal.e2e.ts-snapshots/select-modal-diff-ios-ltr-Mobile-Chrome-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+11.8 KB
...lect-modal.e2e.ts-snapshots/select-modal-diff-ios-ltr-Mobile-Firefox-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+10.9 KB
...elect-modal.e2e.ts-snapshots/select-modal-diff-ios-ltr-Mobile-Safari-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+6.05 KB
...select-modal.e2e.ts-snapshots/select-modal-diff-md-ltr-Mobile-Chrome-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+11.4 KB
...elect-modal.e2e.ts-snapshots/select-modal-diff-md-ltr-Mobile-Firefox-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+10.9 KB
...select-modal.e2e.ts-snapshots/select-modal-diff-md-ltr-Mobile-Safari-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+7.06 KB
...al.e2e.ts-snapshots/select-modal-multiple-diff-ios-ltr-Mobile-Chrome-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+13.1 KB
...l.e2e.ts-snapshots/select-modal-multiple-diff-ios-ltr-Mobile-Firefox-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+12 KB
...al.e2e.ts-snapshots/select-modal-multiple-diff-ios-ltr-Mobile-Safari-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+6.33 KB
...dal.e2e.ts-snapshots/select-modal-multiple-diff-md-ltr-Mobile-Chrome-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+12 KB
...al.e2e.ts-snapshots/select-modal-multiple-diff-md-ltr-Mobile-Firefox-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+11.4 KB
...dal.e2e.ts-snapshots/select-modal-multiple-diff-md-ltr-Mobile-Safari-darwin.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cancelText: this.cancelText,
I consider better to use the existing property cancelText, instead of creating a new one, so we avoid having two properties for the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you @luisbytes. Thanks for letting me know.
I'll change it and push the changes today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @luisbytes 😄