-
Notifications
You must be signed in to change notification settings - Fork 45
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
improvement: use tableV2 in the alert page #3669
Conversation
Hello gaudiauj,My role is to assist you with the merge of this Status report is not available. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
a41bb40
to
1f238dd
Compare
History mismatchMerge commit #a41bb40034d4681aa9aa7f8b0a9e32abda037077 on the integration branch It is likely due to a rebase of the branch Please use the |
/reset |
Reset completeI have successfully deleted this pull request's integration branches. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
ui/src/containers/AlertPage.js
Outdated
const leafAlerts = useMemo( | ||
() => alerts?.alerts.filter((alert) => !alert.labels.children) || [], | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
[JSON.stringify(alerts?.alerts)], | ||
[alerts?.alerts], |
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 we should keep the previous code.
React uses “referential equality” for object and array within the dependencies array.
so even we have the same content of alerts, it will still re-run the effect. That's why we stringify.
A pretty good article to read about.
https://www.benmvp.com/blog/object-array-dependencies-react-useEffect-hook/
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
1f238dd
to
e81431e
Compare
History mismatchMerge commit #1f238dd02845ae341eac4efaeded1e958ab18456 on the integration branch It is likely due to a rebase of the branch Please use the |
e81431e
to
da45c76
Compare
/reset |
Reset completeI have successfully deleted this pull request's integration branches. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
da45c76
to
fe730eb
Compare
History mismatchMerge commit #da45c767229ae18f42868fde130c220d9b23beb5 on the integration branch It is likely due to a rebase of the branch Please use the |
/reset |
Reset completeI have successfully deleted this pull request's integration branches. |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
|
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following reviewers are expecting changes from the author, or must review again: |
const leafAlerts = useMemo( | ||
() => alerts?.alerts.filter((alert) => !alert.labels.children) || [], | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
You will need to keep this line, otherwise Eslint will complain about about the missing dependency :(
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.
oh my bad will fix this :)
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/status |
StatusStatus report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
/approve |
Build failedThe build for commit did not succeed in branch w/123.0/bugfix/alertpage. The following options are set: approve |
/status |
StatusStatus report is not available. The following options are set: approve |
Build failedThe build for commit did not succeed in branch w/123.0/bugfix/alertpage. The following options are set: approve |
fe730eb
to
27ea007
Compare
History mismatchMerge commit #fe730ebb380b349462ee182e6580fee21a0d8a55 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: approve |
/reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye gaudiauj. |
Component:
AlertPage
Context:
fix bad formating of alerts by using new table