Conversation
…rture-stack/lectern into 395-render-dictionary-schema
ethanluc7
changed the base branch from
dependabot/npm_and_yarn/ajv-8.18.0
to
main
joneubank
left a comment
joneubank
left a comment
There was a problem hiding this comment.
Great work here, updates make this much cleaner. Few minor updates to request.
| &:hover { | ||
| ${hoverText} | ||
| } | ||
|
|
||
| &:nth-child(even) { | ||
| background-color: ${theme.colors.accent_1}; | ||
| border-block: 1.5px solid ${theme.colors.accent_2}; | ||
| } | ||
|
|
||
| &:nth-child(even):hover { | ||
| ${hoverText} | ||
| } |
| schema.restrictions.foreignKey.forEach((foreignKey) => { | ||
| const fkIndex = fkRestrictions.length; |
There was a problem hiding this comment.
should this fkIndex match the index form the schema.restrictions.foreignKey array that corresponds with the foreignKey element in the forEach loop that we have at this point? If so, the forEach interface provides the index:
schema.restrictions.foreignKey.forEach((foreignKey, fkIndex) => {Edit after reading the whole code block:
this is trying to match the index for the new entry we push into fkRestrictions, so what you have is correct, and it just so happens that the indexes match since this loop is the only thing modifying the fkRestricitons array. So I think you don't need to change this, but this is a bit strange to see... requires knowing that this other array is being modified from within this loop.
| continue; | ||
| } | ||
| for (const idx of indices) { | ||
| if (!visitedFkIndices.has(idx)) { |
There was a problem hiding this comment.
This is duplicate check performed in visitFk?
you are adding values to Sets... they will never add duplicates so I think you can skip these checks.
| ...edge, | ||
| className: undefined, | ||
| markerStart: ONE_CARDINALITY_MARKER_ID, | ||
| markerEnd: { type: MarkerType.Arrow, width: 20, height: 20, color: '#374151' }, |
There was a problem hiding this comment.
surprising seeing style information baked into the component logic. Should the style information be separated out or moved into a class to add to the elements?
| useEffect(() => { | ||
| setEdges((currentEdges) => getEdgesWithHighlight(currentEdges, activeEdgeIds, theme.colors.secondary_dark)); | ||
| }, [activeEdgeIds, setEdges]); |
There was a problem hiding this comment.
Is this useEffect truly necessary? Is it possible to achieve the same outcome with by assigning edges with a useMemo that has [activeEdgeIds] as its dependencies?
The useEffect will trigger after the first render and then re-render with edge highlights, and I don't think thats needed. We're not doing any kind of work, we're just calculating the state for rendering which I think does not need the useEffect.
2 participants
Related to issue: #397
Summary
Adds interactive highlight states to the Entity Relationship Diagram, allowing users to click on schema nodes to activate/deactivate edges and hover over rows and edges. Also includes a comprehensive set of dictionaries (single schema, linear chains, fan-out, cyclical, compound keys, etc.) for testing various ERD topologies.
Description of Changes
Readiness Checklist
.env.schemafile and documented in the README