-
Notifications
You must be signed in to change notification settings - Fork 15
[DAPS-1408] Provenance Visual Management (2/2) #1419
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
[DAPS-1408] Provenance Visual Management (2/2) #1419
Conversation
stroke: #777; | ||
} | ||
|
||
.nodes .anchored { | ||
stroke-dasharray: 5,3; | ||
stroke-width: 3px; | ||
} | ||
|
||
/* Tooltip for graph interaction help */ | ||
.graph-tooltip { | ||
position: absolute; | ||
bottom: 10px; | ||
left: 10px; | ||
background: rgba(0, 0, 0, 0.7); | ||
color: #fff; | ||
padding: 5px 10px; | ||
border-radius: 3px; | ||
font-size: 12px; | ||
pointer-events: none; | ||
z-index: 1000; | ||
} | ||
|
||
/* Graph context menu */ | ||
.graph-context-menu { | ||
background: #222; | ||
border: 1px solid #444; | ||
border-radius: 3px; | ||
box-shadow: 0 0 10px rgba(0,0,0,0.5); | ||
bottom: 10px; | ||
left: 10px; | ||
background: rgba(0, 0, 0, 0.7); | ||
color: #fff; |
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.
Relevant changes
web/static/panel_graph.js
Outdated
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.
Moved into its own component dir. Another time, we can refactor with
/graph-panel/
├── index.js # Main entry point
├── graph-panel.js # Main graph panel class
├── graph-state.js # State management using observer pattern
├── graph-models.js # Node and link data models
├── graph-ui.js # UI components (modals, context menus)
├── graph-renderer.js # D3 rendering logic
├── graph-utils.js # Utility functions
└── styles/
├── graph-styles.css # Original CSS (referenced)
└── graph-custom.css # Custom styles for new features
web/views/main.ect
Outdated
<script nonce="<%= @nonce %>" type="module" charset="utf-8" src="/components/provenance/panel_graph.js"></script> | ||
<link href="/components/provenance/graph_styles.css" rel="stylesheet" type="text/css"> |
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.
Need to load the styles somehow
.github/workflows/unit-tests.yml
Outdated
@@ -2,10 +2,10 @@ name: Unit-Testing | |||
on: push | |||
jobs: | |||
unit-test: | |||
runs-on: ubuntu-20.04 | |||
runs-on: ubuntu-25.04 |
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.
Was seeing:
https://github.com/ORNL/DataFed/actions/runs/15012716441?pr=1419
This is a scheduled Ubuntu 20.04 retirement. Ubuntu 20.04 LTS runner will be removed on 2025-04-15. For more details, see actions/runner-images#11101
823c2ea
to
d82fde9
Compare
@@ -5,7 +5,7 @@ jobs: | |||
runs-on: ubuntu-24.04 | |||
if: ${{ always() }} | |||
steps: | |||
- uses: actions/checkout@v2 | |||
- uses: actions/checkout@v4 |
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.
Why was this needed?
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.
Original context: #1419 (comment)
- it was a quick win to update this to the standard v4 version
const node = { | ||
id: item.id, | ||
doi: item.doi, | ||
size: item.size, | ||
notes: item.notes, | ||
inhErr: item.inhErr, | ||
locked: item.locked, | ||
links: [], | ||
nodeSize: DEFAULTS.NODE_SIZE, | ||
labelSize: DEFAULTS.LABEL_SIZE, | ||
}; |
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.
Would be nice to have some comments around this about some of the less clear attributes,
I.e. locked, is that a boolean or a position? what is inhErr? What are notes, how is it different from a label, you have size, nodeSize and labelSize it's not clear what size is. And what is doi?
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.
These functions also look like they might be testable because I don't see interactions with the browser window elements directly, could be nice to add some tests.
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.
You are going to love these then https://github.com/ORNL/DataFed/pull/1419/files#diff-278ea613b73f67c782af0be1e27dc1e08b6657cea1759a3446288171d53fc4b0
❤️
Can you summarize which files I should pay the most attention to, it looks like in some cases you have just broken the existing logic up a bit to bring clarity. I don't want to spend to much time on the parts you simply moved around. For instance the graph_schema file, it looks like you split that up, but did you make a lot of logical changes or is that mostly formatting? |
1874896
to
7b84f7f
Compare
2b55a60
to
74f4551
Compare
7b84f7f
to
b369e1e
Compare
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.
Pull Request Overview
This PR implements enhancements for Provenance Visual Management by adding node and label color and anchoring changes as well as introducing a customization modal. Key changes include:
- Integration of new CSS styles and assets for graph visualization and customization.
- Comprehensive unit tests for provenance utility functions and state management.
- Updates to state handling and GitHub workflow configuration.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
web/views/main.ect | Added CSS links for provenance graph and customization modal styles |
web/test/components/provenance/utils.test.js | Added tests for utility functions used in graph management |
web/test/components/provenance/state.test.js | Added tests for provenance state management |
web/static/jquery-ui-light/datafed.css | Updated styles for anchored nodes and graph tooltips (light theme) |
web/static/jquery-ui-dark/datafed.css | Updated styles for anchored nodes and graph tooltips (dark theme) |
web/static/components/provenance/utils.js | Created utility functions for graph operations |
web/static/components/provenance/styles/graph_styles.css | Defined styles for provenance graph visualization |
web/static/components/provenance/styles/customization_modal.css | Defined styles for the customization modal |
web/static/components/provenance/state.js | Implemented state management for provenance graph |
web/static/components/provenance/customization_modal.js | Created logic for the customization modal |
web/package.json.in | Updated test script command |
.github/workflows/unit-tests.yml | Updated GitHub checkout action version |
for (let i in a_node.links) { | ||
link = a_node.links[i]; |
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.
Consider using a for...of loop to iterate over arrays instead of a for...in loop to avoid potential pitfalls with enumerable properties.
for (let i in a_node.links) { | |
link = a_node.links[i]; | |
for (let link of a_node.links) { |
Copilot uses AI. Check for mistakes.
64d6956
to
72c932c
Compare
Need to fix hiding and collpasing a node |
72c932c
to
9bde685
Compare
9bde685
to
7cbd39d
Compare
[DAPS-14xx] Address collapse, expand, hide
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.
Just get the CI passing. From my point of view this is a huge improvement in terms of functionality and a much slicker interface.
50746fe
to
45a40c3
Compare
45a40c3
to
fc945c9
Compare
75795fe
into
feat-DAPS-14xx-provenance-node-label-anchors-lint
* [DAPS-14xx] Move files over and move out svg * [DAPS-1408] Provenance Visual Management (2/2) (#1419) * [DAPS-14xx] Base * [DAPS-14xx] Add styling and tooltips * [DAPS-14xx] Add node and label customization * [DAPS-14xx] Add editor modal on right-click. Draggable model * [DAPS-14xx] Revert * [DAPS-14xx] Seperate files, update styles * [DAPS-14xx] simplify feat * [DAPS-14xx] Expand mocha tests, prettier * [DAPS-14xx] Update checkout version * [DAPS-14xx] Add todo * [DAPS-14xx] Update tests, consts, fix tooltip * [DAPS-14xx] Base: [DAPS-14xx] Address collapse, expand, hide * [DAPS-14xx] THEME, address comments * [DAPS-14xx] Zoom * [DAPS-14xx] JSDOC * Update web/static/jquery-ui-dark/datafed.css Co-authored-by: Joshua S Brown <[email protected]> * Update web/static/jquery-ui-dark/datafed.css Co-authored-by: Joshua S Brown <[email protected]> * Update web/static/jquery-ui-dark/datafed.css Co-authored-by: Joshua S Brown <[email protected]> --------- Co-authored-by: Joshua S Brown <[email protected]>
PR/MR templates:
Ticket
DAPS-1408
Description
Add:
How Has This Been Tested?
Artifacts (if appropriate):
See #1408