ref(aci): add optional connect/disconnect column to connected automations list #91408
Conversation
github-actions
bot
added
the
Scope: Frontend
natemoo-re
left a comment
natemoo-re
left a comment
There was a problem hiding this comment.
Few things to clean up, but looks good otherwise!
static/app/components/workflowEngine/gridCell/automationTitleCell.tsx
Outdated
Show resolved
Hide resolved
| } else { | ||
| newSet.add(id); | ||
| } | ||
| localStorage.setItem('connectedIds', JSON.stringify(Array.from(newSet))); |
There was a problem hiding this comment.
I'd consider either accepting a required key parameter to use as the localStorage key or leveraging the useId() hook to generate one. Currently, interacting with multiple components that use this hook would overwrite earlier states.
There was a problem hiding this comment.
ah yep, i forgot i was still brainstorming solutions for this. i didn't know useId existed, thanks! fixed in f048ee6
There was a problem hiding this comment.
update: i realized we can't just generate a unique key for every use, since we want step 1 and 2 of automation creation to maintain the same list of connectedIds. i've updated this hook to take a key parameter, but made a const key for steps 1 and 2 of automation creation to share (see ee135ba). let me know if there is a better way for 2 views to share a key!
ameliahsu
force-pushed
the
mia/aci/ref-connected-automations-list
branch
from
18d5e17 to
97702ec
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #91408 +/- ##
===========================================
+ Coverage 29.61% 75.94% +46.32%
===========================================
Files 8728 10322 +1594
Lines 487191 585475 +98284
Branches 22549 22527 -22
===========================================
+ Hits 144282 444634 +300352
+ Misses 342227 140420 -201807
+ Partials 682 421 -261 |
ameliahsu
force-pushed
the
mia/aci/ref-connected-automations-list
branch
from
ee135ba to
406cc52
Compare
static/app/views/automations/components/editConnectedMonitors.tsx
Outdated
Show resolved
Hide resolved
adding the option to show a connect/disconnect column to the connected automations list, which will be used for the monitor creation page
refactored the
useConnectedMonitors()hook so that it could be shared between the connected monitors and automations lists