-
Notifications
You must be signed in to change notification settings - Fork 49
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
Refactor to use display entities #698
Conversation
a5f20a5
to
27336ba
Compare
722543b
to
ed9b9a0
Compare
ed9b9a0
to
a66e7c6
Compare
packages/graph-explorer/src/core/StateProvider/displayVertex.ts
Outdated
Show resolved
Hide resolved
packages/graph-explorer/src/core/StateProvider/displayAttribute.ts
Outdated
Show resolved
Hide resolved
5f0ad20
to
0c94baf
Compare
packages/graph-explorer/src/core/ConfigurationProvider/useConfiguration.ts
Outdated
Show resolved
Hide resolved
packages/graph-explorer/src/core/StateProvider/displayAttribute.ts
Outdated
Show resolved
Hide resolved
packages/graph-explorer/src/modules/EntitiesTabular/components/NodesTabular.tsx
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #698 +/- ##
==========================================
+ Coverage 14.88% 18.12% +3.24%
==========================================
Files 566 540 -26
Lines 25121 23249 -1872
Branches 1170 1166 -4
==========================================
+ Hits 3739 4214 +475
+ Misses 21050 18750 -2300
+ Partials 332 285 -47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I've pushed a series of changes:
|
Fixed tests and added some more |
@@ -37,7 +37,7 @@ const EdgeDetail = ({ edge }: EdgeDetailProps) => { | |||
</div> | |||
<div className={"content"}> | |||
<div className={"title"}>{edge.displayTypes}</div> | |||
{edge.displayId && <div>{edge.displayId}</div>} | |||
{edge.hasUniqueId ? <div>{edge.displayId}</div> : null} |
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.
Curious why this check is preferred to the original check for displayId
? Is there still value in setting the displayId
on the EdgeDetail
for SPARQL if it's not shown?
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.
I honestly don't remember why I made that change. If I had to guess it was to make sure no properties were optional on the model. But that's not a good enough reason.
I reverted the change.
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.
Actually, I take it all back. After reverting the change and seeing some errors I now remember why. It is about optionality, but specifically around the logic to get the "display name" value. Since displayId
is an option on non-sparql databases if it was optional it would require display name to be optional, which I do not want.
4299e8c
to
b541601
Compare
This reverts commit b541601.
LGTM 👍 |
Description
Nearly all views that show details of a node, edge, or type would do the exact same transformations at the view level. This change abstracts all of that logic to a single place, ensuring the logic is consistent and making it easier to modify the behavior across the app.
The next step of this refactoring will be to abstract the neighbor count logic to the same
DisplayVertex
. And after that we will have a good foundation for loading a node/edge from a simple ID, which is foundational to URL sharing.Validation
Related Issues
Check List
license.
pnpm checks
to ensure code compiles and meets standards.pnpm test
to check if all tests are passing.Changelog.md
.