Skip to content

test(item-sliding): add test for side issue with CDN #30469

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
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

brandyscarney
Copy link
Member

@brandyscarney brandyscarney commented Jun 10, 2025

Adds test for #29845

Must be merged after the fix in that PR is released. The test will fail while using the older version of the CDN.

Copy link

vercel bot commented Jun 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 10, 2025 2:28pm

@github-actions github-actions bot added the package: core @ionic/core package label Jun 10, 2025
@brandyscarney brandyscarney marked this pull request as ready for review June 10, 2025 14:13
@brandyscarney brandyscarney requested a review from a team as a code owner June 10, 2025 14:13
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Had one question to better understand the approach but non blocking.

Comment on lines 59 to 61
<link href="${baseUrl}/scripts/testing/styles.css" rel="stylesheet" />
${palette !== 'light' ? `<link href="${baseUrl}/css/palettes/${palette}.always.css" rel="stylesheet" />` : ''}
<script src="${baseUrl}/scripts/testing/scripts.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Why not these as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

The testing scripts file is required for Playwright. Without it we would have to set all of the stuff for telling Playwright the app is loaded and determining the document direction, palette, etc: https://github.com/ionic-team/ionic-framework/blob/main/core/scripts/testing/scripts.js

I actually accidentally removed this at first and ran into an issue where none of the test after setContent was executed because window.testAppLoaded = true was never set.

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

github-merge-queue bot pushed a commit that referenced this pull request Jun 10, 2025
…alue (#29845)

Issue number: resolves #29499

---------

## What is the current behavior?
Using the bundled version of Ionic results in an error `Uncaught (in
promise) Error: "undefined" is not a valid value for [side]. Use "start"
or "end" instead.` with `<ion-item-sliding>`.

Reproduction 1: 
- https://stackblitz.com/edit/stackblitz-starters-mezoy6?file=index.html
- Press the Add (`+`) button several times

It is also reproducible in an Ionic Angular app when installed via npm.

Reproduction 2: 
-
https://stackblitz.com/edit/angular-exwgke?file=src%2Fapp%2Fexample.component.ts
- Press the `Add Items` button several times

## What is the new behavior?
Check for the side attribute to avoid `side` being `undefined`.

## Does this introduce a breaking change?
- [ ] Yes
- [x] No

## Other information

Dev build: `8.6.1-dev.11749562115.1e681558`

Test PR: #30469

The tests must be merged separately, after this is released, in order to
use the fix from the CDN.

---------

Co-authored-by: Brandy Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants