add KFocusTrap and use it in Kmodal #764
Conversation
Sure! |
lib/KFocusTrap.vue
Outdated
| export default { | ||
| name: 'FocusTrap', | ||
| props: { | ||
| disabled: { |
There was a problem hiding this comment.
Would you document this prop? You can see other components.
The docstring will then show in the auto-generated props documentation on a documentation page for this component.
lib/KModal.vue
Outdated
| > | ||
| <!-- KeenUiSelect targets modal by using div.modal selector --> | ||
| <KFocusTrap> | ||
| <component :is="wrapper"> |
There was a problem hiding this comment.
We'll need to test that when component here is KOverlay (that is, when KModal's appendToOverlay prop is true), the focus trapping still works as expected.
Just from the code I'm a bit suspicious there may be trouble. Reason being that KOverlay will move the modal content to another element #k-overlay, whereas KFocusTrap will stay in its original place in the DOM.
I know I used this structure as an example in the issue, however it was meant to be just rough guidance rather than solution, apologies if that was confusing. Let's preview for both possible appendToOverlay values and see if it needs to be adjusted.
There was a problem hiding this comment.
oh , i should have understood the requirements properly, thanks @MisRob i will make sure to use KFocusTrap appropriately.
|
Leaving a note here that adding a documentation page for |
|
Thanks @lokesh-sagi125 for another contribution! Just chimed in briefly and left some high-level notes. For more detailed review, I will invite @akolson and also looping in @LianaHarris360 who has had some recent experience with modals and focus trapping. Thanks all :)! |
There was a problem hiding this comment.
I agree with Misha's review so no comments on my side.
We'll however, need to test this and this could include opening a pr in Kolibri for the start. The FocusTrap component is only used in a few places in Kolibri so it could be a good opportunity to do the refactor with the same pr.
|
Ah yes, @akolson, that's a good note. I assigned learningequality/kolibri#12588 to someone else without much thinking but since the other volunteer would need to wait for this PR and we need to test it, if @lokesh-sagi125 is interested we could re-assign the related Kolibri issue. That'd make most sense but of course, depends on the time you wish to dedicate @lokesh-sagi125, no pressure. Alternatively, only one or two places would be tested in Kolibri as part of this work. Whatever works for you, just flagging it here so we don't have two volunteers working on the same thing :) |
Co-authored-by: Michaela Robosova <[email protected]>
|
yes @MisRob i am happy to work on issue learningequality/kolibri#12588 :), pls assign it to me. |
|
Hi @lokesh-sagi125 , please follow the instructions that have been added as code comments in https://github.com/learningequality/kolibri-design-system/blob/7bfa45132cd70df0ddcd950e5062fcd13f234b08/docs/pages/playground.vue |
|
thanks @akolson |
Not sure if you are having issues with the set up also, but installing the dependencies |
@lokesh-sagi125 Great, thank you. Can you post a comment to learningequality/kolibri#12588? Otherwise I can't assign. |
Hi @MisRob, I would love to start the documentation page. |
|
@shivam-daksh I am so sorry I don't know why I tagged you, probably just clicked a bit further. It was meant for @lokesh-sagi125 :) Let's hear from @lokesh-sagi125 at first. However we have some other open documentation issues @shivam-daksh. Thanks a lot. |
|
Or you two can take my typo as a nice opportunity to co-hack, heh :)! Whatever works for you. Let @lokesh-sagi125 as the author choose. |
|
hi @MisRob yes i will start the documentation page, i am very in-experienced in documentation so i would require a little bit of guidance. thank you. |
@lokesh-sagi125 thanks! You're welcome to ask questions as you proceed, however the links in the issue already have plenty of guidance regarding technically creating the page as well as writing guidelines. I'd also recommend you examine documentation pages for other components - the underlying code and writing style too. |
…ed inside KRadioButtonGroup
revert changes made to KModal and remove KFocustrap for the time being
|
@MisRob any idea why this was closed? |
|
I will soon raise another PR for this issue. Apologies for not providing prior notice. |
|
Thank you @lokesh-sagi125! No worries, please let us know if you have any question :). |
Description
copied the focus trap form 'kolibri' and created a new component "KFocusTrap" in the KDS and wrapper 'Kmodal' with it.
Issue addressed
Add KfocusTrap and use it Kmodal
Addresses #746
Changelog