fix(node.icon): Allow Node.icon to inherit Node.size#2444
Conversation
… inherited from `node.size` if unset
|
It does seem that two of the visual regression checks are failing, but that's expected since those have a If we want to add this, but prevent regressions in networks created with the size unaware icons, we could maybe add a new |
…itSize` flag is set at some level of the icon definition
|
Additional commits should preserve original behavior and hide size inheriting behind a new Value add here is that currently, icons that are set in groups can't be dynamically sized without directly copying the group icon object to the node and updating the size. This will allow you to use icons and drive the size the same way you drive the size of every other shape type. Here's a sample graph that this would benefit I wanted to set the icon groups based on file extension, then size the nodes based on line count, but the group icon size is set to 50 and overwrites the node size currently. |
|
If this is something that will be merged, we might want to do the same thing with |
No AI used at any stage of this PR
Change option resolution order for Node to allow node icon size to inherit from node size
I tried to keep the resolution simple. Since
icon.sizeis the only shape that uses a seperatesizeproperty, I had to unset the defaults inNodesHandlerfor bothsizeandicon.size.These options are resolved in
NodeBase.setOptionsnow.Old Icon Size Resolution:
<nodeobj>.icon.size -> <groupobj>.icon.size -> <network.nodes>.icon.sizeNew Icon Size Resolution:
I'm not sure if it's better to have the size be inherited this way though:
If the second option is more logical I will gladly ammend this PR to do it that way. I just figured that
icon.sizebeing directly set at any point should probably supercede thesizefallbacks to prevent issues with existing setups.This might also cause breaking changes if someone is setting
sizeand leaving it set on an icon node because they saw no change. The fix would be to either remove thesizekey from the node or to seticon.sizeat any level of the config.resolves #385