Great job adding back extensibility of node context menus contributed by other extensions #565.
Integration with Visual Studio Code NETCONF extension
Changes made
"contributes": {
"commands": [
{
"command": "netconf.ssh.clabConnect",
"title": "NETCONF",
"icon": "$(vm-connect)"
},
{
"command": "netconf.save.clabAddHost",
"title": "Add NETCONF Device",
"icon": "$(add)"
},
and
"contributes": {
"menus": {
"containerlab/node/context": [
{
"command": "netconf.ssh.clabConnect",
"group": "inline"
},
{
"command": "netconf.ssh.clabConnect",
"group": "access"
},
{
"command": "netconf.save.clabAddHost",
"group": "tools"
}
]
How it renders
Example 1 | NETCONF connect
Example 2 | Add managed NETCONF device
Findings
Groups
The current implementation uses rule-based logic based on the command name to determine the group:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L277-L330
This does NOT work well for external contributions, as someone would need to reverse engineer the mapping rules, while naming the commands accordingly. Right now, the group statement in the package.json is only respected for inline but other values like access or tools are ignored. Therefore I've renamed my commands to contain ".ssh." and ".save" - but this feels wrong!
Icons
For icons, similar applies. But the logic for icons is more advanced:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L566-L588
It tries to figure out first, if it is a supported icon-name based on:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L256-L274
Note:
This is a very restricted subset of codicon values.
If no icon name is provided, or icon name does not match the supported subset, it will try a hard-coded list of command mappings:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L227-L254
Finally if there is still no match, it applies rule-based mappings:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L332-L388
Note:
The webview is not using vsCode codicon but Material UI icons. Some icons like vm-connect don't exist.
https://mui.com/material-ui/material-icons/
Recommendations
- Respect user defined
group settings from package.json.
- Document what groups are supported as contribution points, and guidelines on how contributors should map commands into this structure.
- Validate, choices made for Material UI icons... The linkIcon is quite different from vm-connect. You may consider using Cast or ConnectedTv or AddLink instead.
- Extend the list of supported codicon values. For my extension I am using the
$(add) codicon. That could be mapped to one of the following Material UI icons: Add, AddCircle, AddCircleOutline, AddBookmark, FormatListBulletedAdd, LibraryAdd,PlaylistAdd, or PostAdd.
Final notes
In general, it must be questioned if using MaterialUI makes sense, as it deviates from vsCode style guidelines. My general recommendation would be, to always use vsCode codicon by default - and only for cases where this is no good match to fallback to a different icon-set like Material UI. This is how it could look like in your code:
import AddIcon from '@mui/icons-material/Add';
const exampleMaterialUI = () => (
<div>
<AddIcon />
</div>
);
const exampleCodicon = () => (
<div>
<span className="codicon codicon-add" />
</div>
);
``
Great job adding back extensibility of node context menus contributed by other extensions #565.
Integration with Visual Studio Code NETCONF extension
Changes made
and
How it renders
Example 1 | NETCONF connect
Example 2 | Add managed NETCONF device
Findings
Groups
The current implementation uses rule-based logic based on the command name to determine the group:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L277-L330
This does NOT work well for external contributions, as someone would need to reverse engineer the mapping rules, while naming the commands accordingly. Right now, the
groupstatement in the package.json is only respected forinlinebut other values likeaccessortoolsare ignored. Therefore I've renamed my commands to contain ".ssh." and ".save" - but this feels wrong!Icons
For icons, similar applies. But the logic for icons is more advanced:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L566-L588
It tries to figure out first, if it is a supported icon-name based on:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L256-L274
If no icon name is provided, or icon name does not match the supported subset, it will try a hard-coded list of command mappings:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L227-L254
Finally if there is still no match, it applies rule-based mappings:
https://github.com/srl-labs/vscode-containerlab/blob/main/src/webviews/explorer/containerlabExplorerView.webview.tsx#L332-L388
Recommendations
groupsettings frompackage.json.$(add)codicon. That could be mapped to one of the following Material UI icons: Add, AddCircle, AddCircleOutline, AddBookmark, FormatListBulletedAdd, LibraryAdd,PlaylistAdd, or PostAdd.Final notes
In general, it must be questioned if using MaterialUI makes sense, as it deviates from vsCode style guidelines. My general recommendation would be, to always use vsCode codicon by default - and only for cases where this is no good match to fallback to a different icon-set like Material UI. This is how it could look like in your code: