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

Remove FocusTrap in favor of KFocusTrap #12718

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

lokesh-sagi125
Copy link
Contributor

@lokesh-sagi125 lokesh-sagi125 commented Oct 14, 2024

Summary

I removed all instances of ' FocusTrap ' that were wrapping ' KModal ', as the focus trapping logic is now handled internally within the KModal component. Additionally, I renamed all remaining FocusTrap instances to KFocusTrap, and finally, I deleted the FocusTrap.vue file since it is no longer necessary. These updates ensure focus trapping is handled consistently throughout the system without causing any regressions.

References

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Oct 14, 2024
@MisRob
Copy link
Member

MisRob commented Oct 14, 2024

Thank you, @lokesh-sagi125. Is this ready for review? If so, would you please fill in the PR description and also linked the issue this is resolving? It will help reviewers.

@MisRob
Copy link
Member

MisRob commented Oct 14, 2024

Note related KDS PR learningequality/kolibri-design-system#799. This PR can be merged after a new KDS release with that change is installed in Kolibri.

@github-actions github-actions bot added DEV: renderers HTML5 apps, videos, exercises, etc. APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) labels Oct 14, 2024
@MisRob MisRob marked this pull request as ready for review October 16, 2024 06:57
@MisRob
Copy link
Member

MisRob commented Oct 16, 2024

I am not entirely sure why would the automatic lint affect another files in this commit, but perhaps it's not an issue. @rtibbles may have an idea perhaps.

@MisRob
Copy link
Member

MisRob commented Oct 16, 2024

Thanks for getting the PR description ready @lokesh-sagi125. I think this can be reviewed cc @akolson @nucleogenesis @LianaHarris360.

@MisRob MisRob added the TODO: needs review Waiting for review label Oct 16, 2024
Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

The changes look correct to me but will review this more extensively once learningequality/kolibri-design-system#799 has been approved and released. I suspect the failing tests are to do with it. Thanks @lokesh-sagi125

@akolson akolson changed the title Remove FocusTrap in favor of KFocusTrap [DO NOT MERGE] Remove FocusTrap in favor of KFocusTrap Oct 21, 2024
@AlexVelezLl
Copy link
Member

Hey @lokesh-sagi125! We just released the changes made in learningequality/kolibri-design-system#799, and updated the KDS version in develop with the new release. You can now rebase this PR on top of develop, and run yarn install to fetch the new KDS version 🤗

@lokesh-sagi125
Copy link
Contributor Author

yes @AlexVelezLl will do.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

The updates of FocusTrap -> KFocusTrap look good to me, but there's some misc file changes that look like they might be related to your rebase and/or some linting (ie, reordering of imports, update to .gitignore).

I thought at first it might be related to the automatic linting CI bot that automatically added the commits to your PR but that doesn't appear to be the case.

I think you may need to git fetch --all and git rebase upstream/develop (assuming that git remote -v shows upstream pointing to the learningequality repository -- use whichever remote you have pointing to our repo).

If you don't have our repository setup in your remotes: git remote add upstream https:/github.com/learningequality/kolibri.git then git fetch upstream will get you the latest develop locally, then you can run the git rebase upstream/develop and observe and correct any conflicts that come up.

Please let me know if you have any questions or, if you're a part of our Slack community feel free to reach out in our #dev-community channel for some more direct help

@rtibbles rtibbles added the DEV: Core JS API Changes related to, or to the Core JS API label Nov 5, 2024
@rtibbles rtibbles changed the title [DO NOT MERGE] Remove FocusTrap in favor of KFocusTrap Remove FocusTrap in favor of KFocusTrap Nov 6, 2024
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

As long as tests and linting pass, this should be good to go.

@rtibbles rtibbles dismissed stale reviews from nucleogenesis and akolson November 6, 2024 01:12

Changes addressed.

@rtibbles rtibbles merged commit ad4efd5 into learningequality:develop Nov 6, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Learn Re: Learn App (content, quizzes, lessons, etc.) APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: Core JS API Changes related to, or to the Core JS API DEV: frontend DEV: renderers HTML5 apps, videos, exercises, etc. TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants