Fixed issue #4757 by modying ContextMenuCloak.vue #4958
Conversation
|
Thanks @Arunima22! We will review in the upcoming weeks. I wanted to note that review waiting times are still longer. |
|
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
And this is only a small channel, some can be very large. To resolve this, we could utilize a single |
|
@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:
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. |
| @@ -37,8 +43,14 @@ | |||
| this.y = e.clientY; | |||
| this.setMenu(this._uid); | |||
| }, | |||
| hideMenu(e) { | |||
| if (this.currentContextMenu && !this.$el.contains(e.target)) { | |||
| this.setMenu(''); | |||
There was a problem hiding this comment.
High-level, it seems to me it'd make more sense to setMenu('') from the ContextMenu component in the moment when it's being closed? Since it logically corresponds to the user action.
That could perhaps help with some other issues I raised, and it would improve readability.
Let's chat about why you chose this approach.
There was a problem hiding this comment.
Hello @MisRob Thank you for your reply. I will be happy to experiment with the second approach you suggested where we try to reset global variables from ContextMenu itself.
I was trying to figure out the root cause of this issue before. But why precisely the variables were not resetting in the first place was something I couldn't figure out. I was trying to understand the root of it and tried to isolate the issue as well by deleting the other code but my approach was unsuccessful. One thing that I repeatedly saw was that we were not able to receive the updated value of uid while trying to display the menu. That is why I used a listener object.
There was a problem hiding this comment.
I see - that sounds it may be useful to take a step back and try some thorough debugging one more time, before we decide the final technique to resolve this. It may be some sort of issue with reactivity as well, and those can be a bit tricky. You could try to trace step by step and take some notes about what's exactly happening when the cloak is clicked for the first time, then for the second time, and what's the difference. Feel free to share your findings with code references.
It seems it will be indeed related to resetting those global variables, and it is logical for us to reset them in any case, so now it's just about digging even deeper.
There was a problem hiding this comment.
Thank you, I will work on this in the upcoming week. I will post my notes here.
##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: #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.