Improve #commonMenu menu #16147
Improve #commonMenu menu #16147
Conversation
Vanessa219
requested review from
Vanessa219 and
Copilot
and removed request for
Vanessa219
There was a problem hiding this comment.
Pull Request Overview
This PR refactors menu tracking by replacing data-subname with data-from attributes to better identify menu entry points. The changes introduce more granular tracking constants and update the menu opening logic to include contextual information about where menus are triggered.
Key changes:
- Added a
fromparameter toopenTitleMenu()to track menu origin - Renamed menu source constants from
MENU_DOC_TREE_MORE_*toMENU_FROM_DOC_TREE_MORE_*for consistency - Enhanced
data-fromattribute to include both popover level and specific menu source context
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/src/protyle/header/openTitleMenu.ts | Modified function signature to accept from parameter and moved data-from attribute setting earlier in execution |
| app/src/protyle/header/Title.ts | Updated calls to openTitleMenu() with new constant MENU_FROM_TITLE_PROTYLE |
| app/src/protyle/breadcrumb/index.ts | Updated call to openTitleMenu() with new constant MENU_FROM_TITLE_BREADCRUMB |
| app/src/menus/navigation.ts | Added data-from attribute setting for document tree menu contexts |
| app/src/menus/Menu.ts | Updated comment to clarify data-from attribute purpose |
| app/src/constants.ts | Added new constants for menu sources and renamed existing tree menu constants |
| app/src/block/Panel.ts | Updated condition to use string matching on data-from instead of equality check |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // 移除弹出上使用右键菜单 | ||
| const menuLevel = parseInt(window.siyuan.menus.menu.element.dataset.from); | ||
| if (window.siyuan.menus.menu.element.dataset.from !== "app" && menuLevel && menuLevel >= level) { | ||
| if (menuLevel && menuLevel >= level && window.siyuan.menus.menu.element.dataset.from.includes("popover")) { |
There was a problem hiding this comment.
Potential null reference error. dataset.from may be undefined if the attribute hasn't been set. Add a null check before calling .includes(): window.siyuan.menus.menu.element.dataset.from?.includes('popover')
| if (menuLevel && menuLevel >= level && window.siyuan.menus.menu.element.dataset.from.includes("popover")) { | |
| if (menuLevel && menuLevel >= level && window.siyuan.menus.menu.element.dataset.from?.includes("popover")) { |
这个菜单只处理文档，带上笔记本是会有问题的，我准备在 #15620 把多选选中的笔记本剔除掉，所以是 docs |
|
还有就是我刚刚想起来所有侧栏都是 file-tree，文档树是 sy__file，所以 "tree-xxx" 可能改成 "file-xxx" 更好 |
|
用户选中的，为什么要剔除？选中什么就操作什么，好像没必要限制用户的操作。 一般情况下该类常量的名字和值是保持一致的，否则重复了无法轻易察觉，可能会导致程序逻辑错误。 |
|
因为思源的代码里不会处理笔记本，只会处理文档，但插件不一定能考虑到类型为 docs 的时候参数还能包含笔记本元素，我做插件的时候就发现这里有隐患了。况且思源本身就没有多选笔记本的右键菜单，直接剔除是更好的。 |
|
毕竟是选中的，开发文档补充一下。如果选中的又被剔除，最终数据和实际选中情况相矛盾了。 |
|
正常来讲不应该能同时选中文档和笔记本：#14959 |
|
那应该是修改不同情况下的菜单，而不是取消用户的选中。 |
|
@Vanessa219 我还是认为应该跟事件里的参数保持一致，使用 docs。毕竟就是同一个菜单，没有什么理由不统一。 |
|
不是应该和用户选中的保持一致么？ |
|
事件总线里已经是 docs 了，需要保持开发上的一致性 |
|
那就没传对，元素还包含着用户选中的。 |
|
所以要在事件总线里新加一个 items 吗？ |
|
可以先调整菜单后再统一添加 |
|
我没听懂意思 |
|
分为四种类型的菜单
|
|
要直接改目前的 "open-menu-doctree" 事件的 type 参数吗？然后 data-from 属性值跟这些 type 保持一致 |
|
可以，但目前不改参数，需要先把菜单弄好。否则接口会有问题。 |
|
但严格来说原生没有 “多个笔记本” 和 “包含文档和笔记本” 的场景吧，怎么弄菜单呢？ |
|
多个笔记本可以有关闭和删除菜单。至于其他的可以按照实际情况传递给开发者，给开发者去处理。 |
2 participants
使用 data-from 代替 data-subname #16141 (comment)