-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[RAM] add refresh functionality to alert table #139669
Conversation
Pinging @elastic/response-ops (Team:ResponseOps) |
@maryam-saeidi, can you please check if this PR do what you expected? thanks in advance! |
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.
Awesome! ✨
This will close this ticket: #134892, right?
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @XavierM |
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.
LGTM! Just a small comment
if (!isLoading && refreshNow) { | ||
refresh(); | ||
} | ||
// 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.
nitpick: do we need this eslint-disable
since we're actually referencing refreshNow
in the effect, also does this have a stale reference to isLoading
since it's not a 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.
The array of dependency plays no direct role in ensuring that the effect runs with fresh values (the effect always runs with the value at the time the component is rendered). The array only controls when the effect runs.
https://blog.bitsrc.io/understanding-dependencies-in-useeffect-7afd4df37c96
* add refresh functionality to alert table * add exhaustive deps * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit b1f5030)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
* add refresh functionality to alert table * add exhaustive deps * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit b1f5030) Co-authored-by: Xavier Mouligneau <[email protected]>
* add refresh functionality to alert table * add exhaustive deps * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' Co-authored-by: kibanamachine <[email protected]>
Summary
We need user of the alert table to be able to refresh the alert table when they want. To do that we added a new prop
refreshNow
and we expect this value to be set likenew Date().getTime()
and each time this prop changed, then we will re-run the query with the same parameter.#134892
Checklist