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(runtime): scoped slot append/prepend correct order after interaction #5970

Merged

Conversation

yigityuce
Copy link
Contributor

@yigityuce yigityuce commented Sep 5, 2024

What is the current behavior?

GitHub Issue Number: #5969

What is the new behavior?

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

Other information

@yigityuce yigityuce requested a review from a team as a code owner September 5, 2024 14:26
@tanner-reits tanner-reits self-assigned this Sep 5, 2024
@yigityuce
Copy link
Contributor Author

to summarize, I added s-sh flag to the existing prepend patch and copied that block (s-ol and s-sh updating block) to the appendChild patch as well.

the others (InsertAdjacentHTML, InsertAdjacentText, InsertAdjacentElement, append, etc) are using the prepend or appendChild internally, so they don't need to be updated

@christian-bromann
Copy link
Member

@yigityuce do you think we can add a unit test for this change?

Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

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

@yigityuce Changes look good! Only request would be to add a test like @christian-bromann suggested. You could probably add a Webdriver test to our existing test suite for these methods here. Let me know if you have any questions!

@tanner-reits tanner-reits changed the title fix(dom-extras): add required internal flags to the new nodes to work… fix(runtime): scoped slot append/prepend correct order after interaction Sep 6, 2024
@yigityuce
Copy link
Contributor Author

hey @christian-bromann & @tanner-reits tests are added as a separate test file since to reproduce the issue, I need to remove the default slot wrapper and I didn't want to break the existing test case.

Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

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

@yigityuce Thanks! Everything looks good. I'll get this merged and try to get a release out this week!

@tanner-reits tanner-reits added this pull request to the merge queue Sep 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 9, 2024
@tanner-reits tanner-reits added this pull request to the merge queue Sep 9, 2024
Merged via the queue into ionic-team:main with commit 2569abd Sep 9, 2024
88 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants