-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Added option for a nested context menu #84
Conversation
tdulcet
commented
Nov 12, 2023
- Added option for a nested context menu and enabled it by default. Fixes Option for nested context menu #44
- Enabled the Subscript Unicode font added in Disable autocorrect feature by default #72.
- Enabled the existing readable text and live preview options by default.
We should probably give some thought to this from #44 (comment):
|
Yeah, but IMHO your screenshot looks nice. And thanks, this looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff but thanks!
title: menuText, | ||
contexts: ["editable"] | ||
}); | ||
for (const atransformationId of amenuItems) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not convinced by this a
prefixing again. And here it looks absurd as it is grammatically wrong in English, I mean what is "a menu items"? Plural/singular what?
for (const atransformationId of amenuItems) { | |
for (const currentTransformationId of currentMenuItems) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I ponder whether this could be written/refactored with less indentation in a recursive way or so, just unsure. So e.g. check if the item is a list and then call the function itself to add it to a new list in a sub-item.
This could drastically reduce the code and complexity and make the code more understandable.
As an addition you can then easily build more sub-entries recursively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will remove the a
prefixes.
Regarding refactoring, I am open to suggestions. I doubt there would ever be multiple levels of nesting, as it would then make it too difficult for users to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yeah, recursive is likely more readable anyway, but it's okish… (It's just quote hard to understand currently, so…)
Co-authored-by: rugk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okayish, dunno if we could optimize the readability as said a bit more there, but otherwise okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this looks much more readable!