#3723 Made Add Rule/Add Rule Section Box #3838
Conversation
Zwendle
changed the base branch from
3647-rulesdashboard---ruleset-table
to
feature/rules-dashboard
| interface RuleActionsProps { | ||
| ruleId: string; | ||
| onAdd: (ruleId: string) => void; | ||
| onAdd: (ruleId: string, anchorEl: HTMLElement) => void; |
There was a problem hiding this comment.
a prop shouldn't take in an anchorEl because it tightly couples this component with its parent. Instead, we need to figure out what information the onAdd needs, and extract that from the element before passing it through (if we need it at all)
| const defaultTab = 'edit-rules'; | ||
|
|
||
| const [showAddMenu, setShowAddMenu] = useState(false); | ||
| const [addMenuAnchorEl, setAddMenuAnchorEl] = useState<HTMLElement | null>(null); |
There was a problem hiding this comment.
if I'm understanding this correctly this useState is to keep track of which element the addMenu is on, in which case keeping track of the html element is too tightly coupling this component with its children. Instead I think it might make more sense to just have a single optional useState for which rule to show the addMenu on
| console.log('Add rule to:', ruleId); | ||
| const handleOpenAddMenu = (ruleId: string, anchorEl: HTMLElement) => { | ||
| // trying to make tests run lol this comment can get deleted later | ||
| if (showAddMenu && addMenuAnchorEl === anchorEl) { |
There was a problem hiding this comment.
this should be handled in the child component because it should know whether or not it has a menu open. This will also make it so we don't have to pass the anchor element through at all
Changes
Notes
3647-rulesdashboard---ruleset-tableinto my branch because I wanted the file organization changes that Kinsey did in her/Mimo's pr. Currently set it so that my branch will merge into that branch just so it's easier for you guys to see the changes for this ticket, but I'll make sure my branch merges intofeature/rules-dashboardonce it's approved... hopefully...RuleActionscomponent a bit so it's less abstract now. Aryan let me know if that's okay, if not I might need your help to revert it back to how it was lol sorryTest Cases
Screenshots
Tadaaaa
To Do
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #3723