-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initial draft of relational rule map component #56
Conversation
…ts rule relationships.
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.
This is great! Makes understanding the multi-part rules so much easier. Love how you can see rule information at a glance too. And the embed feature is real cool too.
Some possible UI improvements:
- Can we keep the header the same between both list and graph views? Remove the current link to the "Rule Map" and then put a radio toggle under the search that allows the user to switch between "List View" and "Map View". This is more in line with how Real Estate websites do it. See the screen below that uses a "Segmented" antd element
- I was hoping we could use the same searching and filtering elements between the two views. But I see how that might be tricky for the filtering, since it is part of the list header directly at the moment. Maybe we can switch the search to be the same for both though (like in the screenshot?)
- When loading the page, we should use the same loader animation as used by the list
- I think the arrows are hard to see at the moment - they are hidden by the dot
- Is it possible to reduce crossover of the nodes by default? It feels like I can usually pull the elements into a position where they are not crossing over. I'm not sure how easy this would be, so ignore this if you've already played around with it a bunch and it's not easy.
- Is it possible to weight parent nodes in such a way where they're more likely to be above their child nodes? Like make it look more like a family tree with the parents above the children. Obviously the relationships can be more complicated than a typical family, but I feel like something in this direction would make it easier to process the relationships.
tsconfig.json
Outdated
@@ -21,6 +21,6 @@ | |||
"@/*": ["./*"] | |||
} | |||
}, | |||
"include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts", "app/utils/getFieldValidation.test.js"], | |||
"include": ["next-env.d.ts", "**/*.ts", "**/*.tsx", ".next/types/**/*.ts", "app/utils/getFieldValidation.test.js", "app/utils/graphUtils.test.js"], |
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.
Can we just include "**/*.test.js"
instead of the individual files
app/map/page.tsx
Outdated
const klammRuleData = await getBRERules(); | ||
|
||
// First map Klamm rules with URLs from matching rules | ||
const mappedKlammRules = klammRuleData.map((klammRule) => { | ||
const matchingRule = maxRuleData.data.find((rule) => rule.name === klammRule.name); | ||
return { | ||
...klammRule, | ||
url: matchingRule ? `${matchingRule._id}` : undefined, | ||
filepath: matchingRule ? matchingRule.filepath : undefined, | ||
reviewBranch: matchingRule?.reviewBranch, | ||
isPublished: matchingRule?.isPublished, | ||
}; | ||
}); |
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'm wondering if it's better to do the big giant call like you've done here to get all the Klamm rules and match them or if it's better to just do the calls to Klamm individually when the user clicks on them.
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've separated the logic for this into utilities in the graph utilities file, and I think it works right now as a big call. In order to map all of the nodes, including draft nodes, I need to call both Klamm and the database for all rules at least once, so might as well use the data? Could review in the future if there is a big hangup on this, but I've also added an additional loading screen just for the rule map tab so that loading of the main rules list is not held up waiting for any of the queries to complete. What do you think about this approach?
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.
Yeah, that sounds good to me!
import { useEffect, useRef, useState, RefObject } from "react"; | ||
import { CategoryObject } from "@/app/types/ruleInfo"; | ||
import { RuleMapRule } from "@/app/types/rulemap"; | ||
import styles from "@/app/components/RuleRelationsDisplay/RuleRelationsDisplay.module.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.
This can just be a local path
app/hooks/useRuleGraph.ts
Outdated
svgRef: RefObject<SVGSVGElement>; | ||
dimensions: { width: number; height: number }; | ||
searchTerm: string; | ||
categoryFilter: string | undefined; |
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.
Can use categoryFilter?: string
notation
app/map/page.tsx
Outdated
setLocation(window.location); | ||
}, []); | ||
|
||
const getOrRefreshRuleList = async () => { |
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.
A similar method to this is used 3 times. Could we put it somewhere so it's not repeated?
app/contexts/RuleModalContext.tsx
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.
I think if this is only used for the RuleModal, it should maybe just live next to that code? Alternatively, could just rename this file so it's just a generic ModelContext.
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've moved this into the Rule Map component directly similar to previous handling of context for the github auth provider.
function GraphContent({ rules, svgRef, dimensions, searchTerm, categoryFilter, showDraftRules }: GraphContentProps) { | ||
useRuleGraph({ | ||
rules, | ||
svgRef, | ||
dimensions, | ||
searchTerm, | ||
categoryFilter, | ||
showDraftRules, | ||
}); | ||
|
||
return <svg ref={svgRef} className={styles.svg} />; | ||
} |
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'm not sure I understand this pattern here. Like it is nice to encapsulate this behaviour but it just feels a little confusing or odd to follow to me. Would it be simpler to just have the "useRuleGraph" useEffect code just in a util function and just have the useEffect living directly in RuleRelationsGraph and calling that util function?
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.
Since the code uses the context hook to handle the description box modal, I was experiencing difficulty in implementing it directly in the RuleRelationsGraph due to the hook rules. For improved clarity i could move the logic from the hook file into a new subcomponent 'RuleGraph' which would hold the useRuleGraph hook and the GraphContent functional component? since the GraphContent component is the only place the useRuleGraph hook is utilized, it would make sense to have them together, and then only import the GraphContent for rendering of the actual svg?
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.
Great. Looks more clear to me now
2c53f11
to
815b7b6
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.
Love the changes
New rule map component introduced to visually represent the connections between rules.
This component pulls rule data from Klamm and combines it with BRM App rule data, in order to create a visual diagram of the parent/child connections between rules. As Klamm data is shared between dev and production, this visual diagram currently represents the production version of connections between rules as they are defined in Klamm.
The Rule Relations Display component utilizes three primary React subcomponents - RuleGraphControls, RuleDescription, and DescriptionManager. These manage the interactive controls (search, category, and draft display), graph legend, and rule description popups when links are clicked.
The actual display of the rule 'graph' is managed by dynamically drawing nodes with links in an SVG using d3.js - a new dependency in this project. Graph data is primarily managed through RuleNodesGroup, which renders the nodes and links, and the useRuleGraph hook which renders and manages the state of the SVG. The useRuleGraph hook incorporates the GraphNavigation and RuleFilters functions to manage accessible keyboard navigation and conditional display of nodes and links.
The GraphUtils file provides all the utilities required to populate the graph with connections, including filtering of rules by filepath, category, and search term.
The rulemap has been added as its own discrete page, navigable to from the home page. This version includes the ability to view the entire map of all rules, including search/category filtering. When a search term or category is selected, a user is able to generate an embedded link, which when navigated to shows a limited view of the given search/category.
If a user navigates to a rule, there is also a new button in the rule action bar in the Dev and Production views to 'View Map'. This button will open the map for this specific rule, showing only the other rules and links connected to this rule. This view includes a 'create embedded link' button, so that this view can also be independently embedded if desired.
Clicking on a rule node will show the relationships between the rules - with dependencies recursively mapped through children/parent rules. This allows a user to see what rules will be impacted by changes to any given rule, and how they may interact.
Clicking on the 'name/label' of a node will open a description of the node in an accessible popup. This pulls data from klamm where available, and provides links to the rule in both klamm and the application, if available.