chore: add ImpactMetricRegistrationContext - #11854
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| openRegisterDialog: () => void; | ||
| }; | ||
|
|
||
| const ImpactMetricRegistrationContext = createContext< |
There was a problem hiding this comment.
not super set on the name here
| metrics={metricOptions} | ||
| loading={metadataLoading || settingsLoading} | ||
| /> | ||
| <ImpactMetricRegistrationProvider |
There was a problem hiding this comment.
Could have also achieved the same thing by prop drilling (which is what I initially did), but I'm kinda allergic to it
There was a problem hiding this comment.
Is there a more generic way to solve this, though? We can probably assume that modals should replace one another, and it'd be neat to not have to this over and over again?
Also, we're assuming that we don't want that. Are we sure we don't? (we should check with ux)
thomasheartman
left a comment
thomasheartman
left a comment
There was a problem hiding this comment.
It solves the potential problem, but I wish we could do it in a less imperative fashion or solve the problem in more general terms. Not sure we can, though.
Anyway, I've added some state modeling suggestions that I think are reasonable. Have a look.
| const [createChartDialogOpen, setCreateChartDialogOpen] = useState(false); | ||
| const [registerMetricDialogOpen, setRegisterMetricDialogOpen] = | ||
| useState(false); |
There was a problem hiding this comment.
What happens if they're both true, though? Instead of two independent pieces of state, could we model this as
const [openDialog, setOpenDialog] useState<'createChart' | 'registerMetrics' | null>(null)Or something like that?
| open={registerDialogOpen} | ||
| onClose={() => setRegisterDialogOpen(false)} | ||
| /> | ||
| {!registrationContext && ( |
There was a problem hiding this comment.
I'm having trouble understanding why this is here? What's it doing?
| metrics={metricOptions} | ||
| loading={metadataLoading || settingsLoading} | ||
| /> | ||
| <ImpactMetricRegistrationProvider |
There was a problem hiding this comment.
Is there a more generic way to solve this, though? We can probably assume that modals should replace one another, and it'd be neat to not have to this over and over again?
Also, we're assuming that we don't want that. Are we sure we don't? (we should check with ux)
|
|
3 participants
Adds a context so that
MetricSelector(the impact metric empty state) can determine wether to close the "Create impact metric" dialog and open the new register metric dialog, or just open the new register dialog.On the
/impact-metricspage, both dialogs are wrapped in a provider that coordinates the switch. On the flag page (no provider), the register dialog falls back to the original stacked behavior.Screen.Recording.2026-04-17.at.10.18.28.mov