-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Console] Configure refresh frequency #121874
Conversation
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.
@mibragimov, thanks for your efforts 👍
It has left some minor changes to be done. I'm sure, you'll fix that very quickly )
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/settings_modal.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
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.
Left some initial feedback for now, lets keep the conversation going and we can do a full review after we are all happy with the approach to take 🚀
src/plugins/console/public/application/components/refresh_interval.tsx
Outdated
Show resolved
Hide resolved
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.
Thank for patching this up @mibragimov! Latest changes lgtm, left one small nit that I think could simplify the code a little bit but its a non-blocker 🚀
PS: Could we also update the screenshot of the PR?
|
||
export type AutocompleteOptions = 'fields' | 'indices' | 'templates'; | ||
|
||
const MILLISECONDS_IN_MINUTE = 60000; |
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.
nit: what do you think about replacing these constants and the intervalOptions
definition with something more compact that uses pluralization:
const PRESETS_IN_MINUTES = [1, 5, 10];
const intervalOptions = PRESETS_IN_MINUTES.map(value => ({
value: value * 60000,
text: i18n.translate('console.settingsPage.refreshInterval.timeInterval', {
defaultMessage: '{value} {value, plural, one {minute} other {minutes}}',
values: { value },
}),
}));
compressed | ||
options={intervalOptions} | ||
value={pollInterval} | ||
onChange={(e) => { |
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.
please try to use useCallback hook to avoid extra rerenderings 🙏
Pinging @elastic/kibana-stack-management (Team:Stack Management) |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @mibragimov |
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.
Code LGTM 🚀
@@ -87,13 +89,22 @@ export class Settings { | |||
return this.storage.get('disable_history', DEFAULT_SETTINGS.historyDisabled); | |||
} | |||
|
|||
setPollInterval(interval: number) { | |||
this.storage.set('poll_interval', interval); |
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.
Nit: it would be a great idea to move these identificators ( 'poll_interval', for example) to the constant variables. What do you think about it?
@mibragimov Regarding this point from #119218:
This is actually a separate feature from the user-specified poll frequency. The idea was to provide the user with a toggle that, when enabled, would cause Console to only request the mappings, aliases, and templates when it's loaded, instead of on an interval. Does that make sense? |
Got it. I created a separate PR for this feature #122125 |
* Add configurable poll interval Co-authored-by: Muhammad Ibragimov <muhammad.ibragimov@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Closes #119218