Conversation
…rture-stack/lectern into 395-render-dictionary-schema
| justify-content: space-between; | ||
| transition: background-color 0.2s; | ||
| position: relative; | ||
| background-color: ${isHighlighted ? theme.colors.secondary_1 : isEven ? '#e5edf3' : 'transparent'}; |
There was a problem hiding this comment.
Consider using css selectors for alternating colours instead of JS props:
https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Selectors/:nth-child
| type ActiveRelationshipState = { | ||
| edgeIds: Set<string>; | ||
| fieldKeys: Set<string>; | ||
| schemaChain: string[]; | ||
| } | null; |
There was a problem hiding this comment.
Recommend defining the type without the union to null. That way we can use this type to represent known data, and we can always present union with null if the value could be missing.
| activeEdgeIds: Set<string> | null; | ||
| activeFieldKeys: Set<string> | null; | ||
| activeSchemaNames: Set<string> | null; | ||
| activeSchemaChain: string[] | null; |
There was a problem hiding this comment.
Why union with null instead of just using optional fields?
TS (and the underlying JS) is built to use undefined for missing values with syntax like: activeEdgeIds?: Set<string>
One reason to define it as you have done is if you want to ensure that any variable ActiveRelationshipContextValue has the property declared (even if the value is null). The syntax I suggest above would allow a developer to omit the property when the value is not available.
| activeEdgeIds: activeState?.edgeIds ?? null, | ||
| activeFieldKeys: activeState?.fieldKeys ?? null, |
There was a problem hiding this comment.
Defining the type of activeEdgeIds as an optional field would simplify this assignment to not require the ?? null, without any loss of information.
| const foreignFieldKeys: string[] = []; | ||
|
|
||
| foreignKey.mappings.forEach((mapping) => { | ||
| const edgeId = `${schema.name}-${mapping.local}-to-${foreignKey.schema}-${mapping.foreign}`; |
There was a problem hiding this comment.
Hyphens are allowed characters in schema and field names. There are some very rare edge cases where this ID will not be unique. Lectern disallows . characters in field and schema names, perhaps you could use that as the separator in this ID.
| /** | ||
| * Traces the full FK chain from a clicked edge or field, following parent links upward | ||
| * and child links downward to collect all connected edges and field keys. | ||
| * | ||
| * When `mappingIndex` is provided (edge click), only the specific mapping's edge and | ||
| * field pair are collected, and chains are traced through individual fields. | ||
| * When `mappingIndex` is omitted (field click), all mappings of the FK are collected. | ||
| * | ||
| * @param {number} fkIndex — The index into fkRestrictions for the clicked FK | ||
| * @param {RelationshipMap} map — The FK adjacency graph | ||
| * @param {number} [mappingIndex] — Optional index of the specific mapping within the FK | ||
| * @returns {{ edgeIds: Set<string>, fieldKeys: Set<string>, schemaChain: string[] }} All edges and fields in the chain | ||
| */ | ||
| export function traceChain( |
| } | ||
| }; | ||
|
|
||
| const traceFieldDown = (localFieldKey: string) => { |
There was a problem hiding this comment.
missing comments like trace field up has. Since these functions have side effects, let's clearly state what data they modify.
| } | ||
| }; | ||
|
|
||
| const clickedFk = map.fkRestrictions[fkIndex]; |
There was a problem hiding this comment.
This function may be called by a click event handler, but itself has no knowledge or interest in the event that called it. Therefore, we don't know anything about clicks, and shouldn't have a variable with the out of context name clickedFk.
The source of this issue may be the argument name fkIndex. Perhaps making this name more clear like chainStartingIndex then this variable can be given a contextual name like chainStartingFK.
Feel free to use more clear language than I am proposing.
| }; | ||
|
|
||
| dictionary.schemas.forEach((schema) => { | ||
| if (!schema.restrictions?.foreignKey) return; |
There was a problem hiding this comment.
Minor but its safer to do the following instead of one-liners, this also applies to the other instances in the PR:
| if (!schema.restrictions?.foreignKey) return; | |
| if (!schema.restrictions?.foreignKey) { | |
| return; | |
| } |
| export function ActiveRelationshipProvider({ relationshipMap, children }: ActiveRelationshipProviderProps) { | ||
| const [activeState, setActiveState] = useState<ActiveRelationshipState>(null); | ||
|
|
||
| const activateRelationship = useCallback( |
There was a problem hiding this comment.
i appreciate the usage of useCallback here 🔥
3 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