-
Notifications
You must be signed in to change notification settings - Fork 1
EAGLE-1429: Fields on Construct component should not be ports #851
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
Conversation
…event handler
…ruct nodes. Minor spelling mistakes.
Reviewer's Guide by SourceryThis pull request ensures that fields on Construct components cannot be ports. It achieves this by limiting the available usage options in the UI and re-checking the graph after changes to field usage. The UI was updated to use the correct class name for the 'use as' dropdown. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @james-strauss-uwa - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider debouncing the call to
eagle.checkGraph()
infieldUsageChanged()
to avoid excessive graph checks.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… selectedNode() is not an Application
@M-Wicenec rightly pointed out that it doesn't make much sense for the "Use As" dropdown to contain only one option for Construct nodes. Every field on a construct node must have "Use As" set to "No Port". So, I've now hidden that column when a Construct is selected. This follows the same pattern as the "Encoding" column, which is hidden unless the selectedNode is an Application. I've also hidden those two columns in the "columns visibility" drop down, in the cases where the selectedNode doesn't allow that column to be displayed anyway. This is ready to re-test and review. |
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.
yep, looks good!
checkGraph() function already picked up this issue. Construct nodes have a maximum of zero ports. But the issue was only detected when the graph was re-checked, which wasn't happening.
So I added a call to eagle.checkGraph() inside the existing fieldUsageChanged() event handler to re-check graph after change to field usage.
Also update the options data-bind in the UI for the "usage" column in the ParameterTable. If the node is a construct, then only the "NoPort" option appears in the usage drop-down.
Summary by Sourcery
Modify field usage handling for Construct nodes to prevent port creation and ensure graph consistency
Bug Fixes:
Enhancements:
Tests: