Unify breadcrumbs behaviour with other Grafana Apps and main core #3003
Conversation
maskin25
added
the
pr:no public docs
| @@ -89,6 +93,12 @@ class IncidentPage extends React.Component<IncidentPageProps, IncidentPageState> | |||
| store.alertGroupStore.updateSilenceOptions(); | |||
| } | |||
|
|
|||
| componentWillUnmount(): void { | |||
There was a problem hiding this comment.
Wouldn't it be better to make a generic React component that handles this cleanup so that we rely on base class functionality strictly for context usage?
Like a BaseContext<P, S> component that extends React.Component<P, S> where you add the contextType, context, cleanup function and componentWillUnmount (and if ever we're going to add more to the componentWillUnmount for the extended component we can just call cleanup on the base class instead).
There was a problem hiding this comment.
I removed context usage from this PR, I think it is redundant in this case
maskin25
deleted the
maxim/#1906-Unify-breadcrumbs-behaviour-with-other-Grafana-Apps
branch
| @@ -20,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ### Added | |||
|
|
|||
| - Notify user via Slack/mobile push-notification when their shift swap request is taken by @joeyorlando ([#2992](https://github.com/grafana/oncall/pull/2992)) | |||
| - Unify breadcrumbs behaviour with other Grafana Apps and main core# ([1906](https://github.com/grafana/oncall/issues/1906)) | |||
There was a problem hiding this comment.
@maskin25 can you retroactively update the CHANGELOG to remove this line? Seems to be duplicated, correct?
) # What this PR does Unify breadcrumbs behaviour with other Grafana Apps and main core ## Which issue(s) this PR fixes #1906 ## Checklist - [ ] Unit, integration, and e2e (if applicable) tests updated - [ ] Documentation added (or `pr:no public docs` PR label added if not required) - [ ] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required)
What this PR does
Unify breadcrumbs behaviour with other Grafana Apps and main core
Which issue(s) this PR fixes
#1906
Checklist
pr:no public docsPR label added if not required)CHANGELOG.mdupdated (orpr:no changelogPR label added if not required)