studio icon indicating copy to clipboard operation
studio copied to clipboard

Fixed issue #4757 by modying ContextMenuCloak.vue

Open Arunima22 opened this issue 9 months ago • 5 comments

##Summary Added an event listener to ContextMenuCloak such that menu variables are reset on right click and menu is displayed.

##References In response to issue: https://github.com/learningequality/studio/issues/4757

##Reviewer guidance Reviewer can look through the code for file kolibri_studio/contentcuration/contentcuration/frontend/shared/views/ContextMenuCloak.vue where an eventlistener object has been added to reset variables. Reviewer can go to a dummy channel and repeatedly right click on the channel resources to make sure context menu is displayed every time.

Arunima22 avatar Mar 14 '25 17:03 Arunima22

Thanks @Arunima22! We will review in the upcoming weeks. I wanted to note that review waiting times are still longer.

MisRob avatar Mar 17 '25 09:03 MisRob

Hi @Arunima22, thanks for the fix. I was able to reproduce the bug and confirm this helps.

However, I think we need to further improve the approach so that it doesn't add 'click' event listener to each ContextMenuCloak instance. I worry that this will have negative impact on performance - there can be many many instances of the menu cloak on a single page and therefore many listeners:

Screenshot from 2025-04-03 16-38-04

And this is only a small channel, some can be very large.

To resolve this, we could utilize a single 'click' listener taking care of all instances. I recalled one possible approach we used in a different component to deal with something similar - see code around isListenerAdded here. perhaps it can inspire some of this work. In the context of Studio, I am quite not sure what would be the most appropriate location for this - feel free to experiment, or maybe @AlexVelezLl @bjester have some additional ideas?

MisRob avatar Apr 03 '25 14:04 MisRob

@Arunima22 alternatively, I wonder if you've explored an approach that wouldn't require a new listener at all? I haven't really looked deep into the code, just from what I've seen high-level there may be to possible ways:

  • (1) Utilize ContextMenuCloak (that's what you do now)
  • (2) Rather update ContextMenu to reset the global variables (or make it react properly even if those variables are not reset), and if possible without having to add a new listener

Again I don't know if (2) will be possible, and I am aware we use Vuetify components under the hood that may limit our options - just offering this for consideration in case it'd show to be simpler than adding a new listener.

MisRob avatar Apr 03 '25 14:04 MisRob

Hi @Arunima22, are you still planning to resolve feedback?

MisRob avatar May 22 '25 03:05 MisRob

Hello, yes I am, just not getting enough time due to internship commitments. Will look through this on the weekend.

On Thu, 22 May, 2025, 9:04 am Michaela Robosova, @.***> wrote:

MisRob left a comment (learningequality/studio#4958) https://github.com/learningequality/studio/pull/4958#issuecomment-2899797738

Hi @Arunima22 https://github.com/Arunima22, are you still planning to resolve feedback?

— Reply to this email directly, view it on GitHub https://github.com/learningequality/studio/pull/4958#issuecomment-2899797738, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHJLLWO42P3MQFMQTNSEKOL27VAS7AVCNFSM6AAAAABZBGC63OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQOJZG44TONZTHA . You are receiving this because you were mentioned.Message ID: @.***>

Arunima22 avatar May 22 '25 08:05 Arunima22

Hi @Arunima22, since it's been few months, I will close this PR now but if you've already made some progress, just message me and I can re-open. That said, we're now more intentional in planning suitable issues for the community and if you'd like to contribute, I'd recommend this project.

MisRob avatar Jun 23 '25 06:06 MisRob