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

KModal appendToRoot changed to appendToOverlay #12659

Conversation

nucleogenesis
Copy link
Member

Summary

Previously changed from appendToBody to appendToRoot 671ccf1

@MisRob pointed out these should be appendToOverlay now instead.

References

Tests on #12651 should pass after this is merged and that dependabot branch is rebased.

Also is blocking #12571 to some degree so it should be rebased when this is merged.

Reviewer guidance

  • Tests pass?

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend labels Sep 12, 2024
@MisRob
Copy link
Member

MisRob commented Sep 13, 2024

Thank you @nucleogenesis for taking care of this. It may make more sense to push to #12651 so we can merge together with the KDS version bump that requires this change.

As mentioned, this update is needed too but I worry it won't resolve the tests failure. But if we push related changes to #12651, we can then observe and test more easily.

@MisRob
Copy link
Member

MisRob commented Sep 13, 2024

Other than that, appendTo.. update looks correct to me. I will do manual QA as soon as it's moved to #12651 (QA is another benefit of having the version bump together with the changes it requires)

@nucleogenesis
Copy link
Member Author

The changes here have been applied in #12651 @MisRob

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants