You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org> on 2023/06/19 17:44:22 UTC
[GitHub] [superset] Abhishek-kumar-samsung opened a new pull request, #24449: feat: custom refresh frequency
Abhishek-kumar-samsung opened a new pull request, #24449:
URL: https://github.com/apache/superset/pull/24449
Earlier at dashboard level we had very limited options in 'Set auto-refresh interval' as shown in image below
But as per our organisation level we wanted the frequency to be something else that was not in dropdown, so we implemented that.
As part of this PR, we can add custom frequency as required by user, user can select any frequency in hour, minute and seconds and set it as auto refresh interval frequency (in seconds).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1495228835
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +99,51 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ if (value === -1) {
+ this.setState({
+ custom_block: true,
+ });
+ } else {
+ this.setState({
+ custom_block: false,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (refreshIntervalOptions.length === 0) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+ refresh_options.push({
+ value: refreshIntervalOptions[0][0],
+ label: t(refreshIntervalOptions[0][1]),
+ });
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ for (let i = 1; i < refreshIntervalOptions.length; i += 1)
+ refresh_options.push({
+ value: refreshIntervalOptions[i][0],
+ label: t(refreshIntervalOptions[i][1]),
+ });
+ return refresh_options;
+ }
Review Comment:
Yes i kept custom options in the array options that we are showing in dropdown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1756379715
@yousoph Ephemeral environment spinning up at http://52.12.12.69:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1934023295
Hello @rusackas , @geido , @eschutho , i have removed the use of DOM element to hide/show custom view.
Now i have made an element custom_block whose state is determining the visibility of custom view.
Please review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2047857845
@geido can you check your requested changes to (potentially) unblock the merge when you have a chance, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2046584091
Ephemeral environment shutdown and build artifacts deleted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2033474686
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #24449: feat: custom refresh frequency
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1653777248
> > Sorry for the churn and delays on this, but I think the main issue is UI design, not the technical implementation.
> > My own opinion: • It would be nice if it had the Select menu (including the Custom option, rather than a button for Custom) • If (and only if) the Custom option is selected from the Select menu, would the additional fields appear (or disappear) • The Select menu would remain present... if you select something other than Custom, the form elements are removed, and the values ignored.
> > As a side note, I feel like the Submit button should be on the right, and the cancel button on the left. I'm not sure if we have a solid documented pattern around this, though (cc. @kasiazjc)
>
> Okay i will do some modification with UI.
Thanks and sorry for the long wait! What I'm thinking in terms of UI improvements is this:
-> instead of button/switcher use single select, for selecting the type of interval, the options are:
- don’t refresh
- custom interval (triggers the additional fields as shown below)
- all of the preset options that are available today (in the example dropdown I listed only two, but should be all of them)
-> set fixed height for the modal to accommodate for disappearing custom fields, so that the height does not jump
<img width="1108" alt="image" src="https://github.com/apache/superset/assets/36897697/2bda1803-9e41-4306-83e1-ac29f8dc56d9">
I think this way setting up the custom interval will be more intuitive, as we are using common patterns. What do you think and could you possibly take this up @Abhishek-kumar-samsung ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1353495822
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +157,89 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
+ visibility: refreshFrequency === -1 ? 'visible' : 'hidden',
+ display: 'flex',
+ gap: '3%',
+ marginTop: '15px',
+ }}
+ id="custom_block_view"
Review Comment:
Hi, instead of using css to hide/show can we use React to conditionally render the element?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1359476548
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +91,52 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ const custom_block = document.getElementById('custom_block_view');
Review Comment:
while implementing this, i was not getting how to hide/unhide custom options from react so i implemented onclick to hide/unhide dom element.
Not sure how can i do without dom element, but i will check and do if possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1930461161
Hello @Abhishek-kumar-samsung just checking in here. Let me know if you need any support to get this to the finish line. Thanks for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2011316130
I agree we don't want to grow the scope of this PR. We should merge this first, and then deal with the cron idea (which is a nice idea) as a subsequent PR.
For the cron feature, it seems like a reasonable plan to seek "lazy consensus" on the dev@ list. Then if that starts an argument, we can upgrade the conversation to a SIP and vote it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #24449:
URL: https://github.com/apache/superset/pull/24449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #24449: feat: custom refresh frequency
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1655890731
> @kasiazjc
>
> Yes i will take this, i will try to make as you have suggested. I was busy in last few days, so i will work on this from now.
Great, thank you! If you need some more design specs let me know. I was using default font sizes and components that we use in Superset :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1654182440
@kasiazjc
Yes i will take this, i will try to make as you have suggested.
I was busy in last few days, so i will work on this from now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1601276626
@john-bodley @craig-rueda Please check and review.
I have shown only one refresh change option at a time as requested by @john-bodley and removed extra submit button as requested by @craig-rueda
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1757536331
> Hi @kasiazjc I added dropdowns in minute and seconds as you have suggested. Instead of select i used input type with datalist. In datalist i have provided options from 0-59, if user puts some other value and submits then it will display error message.
>
> datalist is looking little different depending on browsers, some previews are as below.
>
> **edge browser preview:** 
>
> **firefox browser preview:** 
>
> **chrome browser preview:** 
>
> I was having problem taking screenshot with dropdown, so i took screenshot using phone.
>
> Please check and review @kasiazjc @rusackas
Sorry for the super late review! Missed this PR. These are not ant design components right? I think single select with predefined list in the dropdown we have in code should be the one used in this for consistency (design and patterns). You can find it in filters for example, and users can also type in some numbers (it just would not accept the new number)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1495575011
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +99,51 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ if (value === -1) {
+ this.setState({
+ custom_block: true,
+ });
+ } else {
+ this.setState({
+ custom_block: false,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (refreshIntervalOptions.length === 0) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+ refresh_options.push({
+ value: refreshIntervalOptions[0][0],
+ label: t(refreshIntervalOptions[0][1]),
+ });
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ for (let i = 1; i < refreshIntervalOptions.length; i += 1)
+ refresh_options.push({
+ value: refreshIntervalOptions[i][0],
+ label: t(refreshIntervalOptions[i][1]),
+ });
+ return refresh_options;
+ }
Review Comment:
Ok then please consider using `map` rather than the for loop to make the code more readable and concise. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1961820984
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1961835267
@geido Ephemeral environment spinning up at http://54.149.118.241:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1936271215
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1664396140
@kasiazjc i implemented the design as you have suggested, Please review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on pull request #24449: feat: custom refresh frequency
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1599186197
@kasiazjc would you mind looking at the design? Personally I think that custom (and how it is defined/configured) should be a secondary option (hidden by default)—possibly under a collapsed menu, button, tab, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1599293767
Okay sure @john-bodley @craig-rueda
I will try to hide custom view and show custom view when required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1936992399
@Abhishek-kumar-samsung Ephemeral environment creation is currently limited to committers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1961683193
> Thanks for the contribution! There are some important changes required from a first-pass review. Let me know when you are ready for another round and I will be happy to have another look
@geido
Can you pls have another look, i resolved all the things that you had mentioned.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2046586087
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24449?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `40.00000%` with `24 lines` in your changes are missing coverage. Please review.
> Project coverage is 67.41%. Comparing base [(`9ced255`)](https://app.codecov.io/gh/apache/superset/commit/9ced2552dbeeaf60217b385d4c40cbaf4372c787?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`4bd1582`)](https://app.codecov.io/gh/apache/superset/pull/24449?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 226 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/superset/pull/24449?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../src/dashboard/components/RefreshIntervalModal.tsx](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&filepath=superset-frontend%2Fsrc%2Fdashboard%2Fcomponents%2FRefreshIntervalModal.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL1JlZnJlc2hJbnRlcnZhbE1vZGFsLnRzeA==) | 40.00% | [23 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #24449 +/- ##
==========================================
+ Coverage 67.34% 67.41% +0.07%
==========================================
Files 1909 1910 +1
Lines 74623 74741 +118
Branches 8324 8356 +32
==========================================
+ Hits 50256 50388 +132
+ Misses 22314 22300 -14
Partials 2053 2053
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/24449/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/24449/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.37% <40.00%> (+0.16%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/24449?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2046583583
> > Hi all - please direct me to an issue link if there's a separate place for questions. We recently adopted Superset and I'm looking into setting up refresh schedules for our dashboards. This PR comes close to introducing the feature I think we need, but really something with the flexibility like cron schedule expressions would be ideal. I haven't been able to understand when the refresh interval triggers (i.e. how it's determined when it'll happen) or to specify a time it should kick off.
> > Effectively I just want to specify when my refresh interval should start and (based off the time window I choose) when it should stop. Can any help me understand this, please?
>
> Hi there, cron refresh would be an ideal solution to tell which dashboard should refresh all their pages at which time, based on the refresh schedule of underneath dataset. Hi @rusackas I wonder is it ideal similar to the cache warm up feature, could you please take a look at this idea or fix the cache warm up feature, so that users will always be able to view the lastest update data without clicking into 'Force refresh' on charts or 'Refresh dashboard' on top. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2079226563
> Thanks @Abhishek-kumar-samsung - I/we appreciate you sticking with this and getting it through!
Thankyou @rusackas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1664390995
> @Abhishek-kumar-samsung my dashboard frequency is saving only for a particular session , how can i make it persistent?
@Shisir99 i am sorey, i was not sure that time, yes it is not persistent for me too, that is for a particular session only.
@rusackas isn't that a problem?
Automatic refresh interval for just a single session does not makes much sense(according to me, or maybe i missed something), it should be persistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1666037051
@rusackas @justinpark @mistercrunch @villebro @geido @eschutho @betodealmeida @nytai @craig-rueda @kgabryje @dpgaspar @kasiazjc @john-bodley
Please review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1765338361
@yousoph Ephemeral environment spinning up at http://54.212.14.85:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1765333595
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1890656507
@kasiazjc @rusackas pls review, its long time since no reply.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1494742341
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +99,51 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ if (value === -1) {
+ this.setState({
+ custom_block: true,
+ });
+ } else {
+ this.setState({
+ custom_block: false,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (refreshIntervalOptions.length === 0) {
Review Comment:
```suggestion
if (!refreshIntervalOptions.length) {
```
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +99,51 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ if (value === -1) {
+ this.setState({
+ custom_block: true,
+ });
+ } else {
+ this.setState({
+ custom_block: false,
+ });
+ }
+ }
Review Comment:
```
this.setState({
custom_block: value === -1,
});
```
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +170,81 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
+ visibility: custom_block === true ? 'visible' : 'hidden',
+ display: 'flex',
+ gap: '3%',
+ marginTop: '15px',
+ }}
+ id="custom_block_view"
+ >
+ <div style={{ width: '30%', margin: 'auto' }}>
+ <FormLabel>
+ <b>{t('HOUR')}</b>
+ </FormLabel>{' '}
+ <br />
+ <input
Review Comment:
Please use the Input provided by Antdesign in src/components
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +99,51 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ if (value === -1) {
+ this.setState({
+ custom_block: true,
+ });
+ } else {
+ this.setState({
+ custom_block: false,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (refreshIntervalOptions.length === 0) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+ refresh_options.push({
+ value: refreshIntervalOptions[0][0],
+ label: t(refreshIntervalOptions[0][1]),
+ });
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ for (let i = 1; i < refreshIntervalOptions.length; i += 1)
+ refresh_options.push({
+ value: refreshIntervalOptions[i][0],
+ label: t(refreshIntervalOptions[i][1]),
+ });
+ return refresh_options;
+ }
Review Comment:
I think you need to review these logics here. Are you trying to put the custom interval at a specific position of the array?
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +157,89 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
+ visibility: refreshFrequency === -1 ? 'visible' : 'hidden',
+ display: 'flex',
+ gap: '3%',
+ marginTop: '15px',
+ }}
+ id="custom_block_view"
Review Comment:
What is that is not working specifically? We definitely need to show this conditionally and not hidden via CSS
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +170,81 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
+ visibility: custom_block === true ? 'visible' : 'hidden',
+ display: 'flex',
+ gap: '3%',
+ marginTop: '15px',
+ }}
+ id="custom_block_view"
+ >
+ <div style={{ width: '30%', margin: 'auto' }}>
+ <FormLabel>
+ <b>{t('HOUR')}</b>
+ </FormLabel>{' '}
+ <br />
+ <input
+ type="number"
+ min="0"
+ className="form-control input-sm"
+ id="custom_refresh_frequency_hour"
+ placeholder="Type a number"
Review Comment:
This needs to be localized `t('Type a number')`
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +99,51 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ if (value === -1) {
+ this.setState({
+ custom_block: true,
+ });
+ } else {
+ this.setState({
+ custom_block: false,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (refreshIntervalOptions.length === 0) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+ refresh_options.push({
+ value: refreshIntervalOptions[0][0],
+ label: t(refreshIntervalOptions[0][1]),
+ });
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ for (let i = 1; i < refreshIntervalOptions.length; i += 1)
+ refresh_options.push({
+ value: refreshIntervalOptions[i][0],
+ label: t(refreshIntervalOptions[i][1]),
+ });
+ return refresh_options;
+ }
+
+ min_sec_options(min_or_sec: string) {
+ const options = [];
+ for (let i = 0; i < 60; i += 1)
+ options.push({
+ value: i,
+ label: `${i} ${min_or_sec}`,
+ });
+ return options;
}
Review Comment:
```
min_sec_options(min_or_sec: string) {
return Array.from({ length: 60 }, (_, i) => ({
value: i,
label: `${i} ${min_or_sec}`,
}));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1985657686
@geido can you pls review again :)
I have made modifications as asked by you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2074005750
> @Abhishek-kumar-samsung it seems that CI is stuck. Please pull latest master and push back to kick CI on a fresh master state. Thank you
Hi @geido i synced my fork to latest main branch, i don't know why the pull_request check is failing.
From code side it did not gave any check failure.
Last time when it ran, the same code passed all checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2033482292
@rusackas Ephemeral environment spinning up at http://54.245.47.10:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1763034395
![Uploading IMG_1487.jpeg…]()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1359476058
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +157,89 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
+ visibility: refreshFrequency === -1 ? 'visible' : 'hidden',
+ display: 'flex',
+ gap: '3%',
+ marginTop: '15px',
+ }}
+ id="custom_block_view"
Review Comment:
I am changing views based on dropdown selected options, i was trying by using react but it was not working,
I am not sure about this, but i will check and try to do if possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1936490888
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1939258090
@geido Ephemeral environment spinning up at http://18.236.100.103:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1752000366
@kasiazjc @rusackas can you pls check the PR, there's been a long time since nobody reviewed it.
If any problem is there then pls tell.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1756375585
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "maanwater (via GitHub)" <gi...@apache.org>.
maanwater commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1961319984
Do you think this will make it to the 4.0.0 release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1507778658
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +169,78 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
Review Comment:
that custom block visibility depends on whether the custom interval option is selected or not, what else can be done to render it conditionally?
from react i am changing the state of custom_block flag, and if it is true then the block gets rendered else not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2046586892
I accidently closed the PR and I reopened it again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung closed pull request #24449: feat: custom refresh frequency
URL: https://github.com/apache/superset/pull/24449
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "LeoDiep (via GitHub)" <gi...@apache.org>.
LeoDiep commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2044155114
> Hi all - please direct me to an issue link if there's a separate place for questions. We recently adopted Superset and I'm looking into setting up refresh schedules for our dashboards. This PR comes close to introducing the feature I think we need, but really something with the flexibility like cron schedule expressions would be ideal. I haven't been able to understand when the refresh interval triggers (i.e. how it's determined when it'll happen) or to specify a time it should kick off.
>
> Effectively I just want to specify when my refresh interval should start and (based off the time window I choose) when it should stop. Can any help me understand this, please?
Hi there,
cron refresh would be an ideal solution to tell which dashboard should refresh all their pages at which time, based on the refresh schedule of underneath dataset.
Hi @rusackas I wonder is it ideal similar to the cache warm up feature, could you please take a look at this idea or fix the cache warm up feature, so that users will always be able to view the lastest update data without clicking into 'Force refresh' on charts or 'Refresh dashboard' on top.
Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1641951247
> Sorry for the churn and delays on this, but I think the main issue is UI design, not the technical implementation.
>
> My own opinion: • It would be nice if it had the Select menu (including the Custom option, rather than a button for Custom) • If (and only if) the Custom option is selected from the Select menu, would the additional fields appear (or disappear) • The Select menu would remain present... if you select something other than Custom, the form elements are removed, and the values ignored.
>
> As a side note, I feel like the Submit button should be on the right, and the cancel button on the left. I'm not sure if we have a solid documented pattern around this, though (cc. @kasiazjc)
Okay i will do some modification with UI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1597546540
@rusackas @justinpark @mistercrunch @villebro @geido @eschutho @betodealmeida @nytai @craig-rueda @kgabryje @dpgaspar
Please review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] craig-rueda commented on pull request #24449: feat: custom refresh frequency
Posted by "craig-rueda (via GitHub)" <gi...@apache.org>.
craig-rueda commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1599189033
From the screenshots, it looks a little confusing to show the dropdown alongside the text fields. Perhaps a new "virtual" option (`CUSTOM`) should be added, which would then hide/show the text boxes when selected? Also, having two buttons when choosing a custom refresh interval looks a little confusing as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1766766398
> 
This looks great, thank you! One last request - could you also change "hours" just to normal input? I think there doesn't need to be any limit and user can type in any number. And the same with styling - if you could use input field that we already have in the code that would be great :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1952715404
Hello @Abhishek-kumar-samsung I am observing some behaviours that should be fixed. In the video you can see the following:
- When going from an option to another, the initial values that I did set for the custom options are not cleared
- When moving from a custom option to a standard option, the values will stay on screen for a short blink and then disappear
https://github.com/apache/superset/assets/60598000/cb53640a-b8e0-4580-a22b-a41f662be3bb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1953466747
Thankyou for reviews @geido
I will check and correct the things.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "LyleScott (via GitHub)" <gi...@apache.org>.
LyleScott commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1746218018
This would be a welcome addition, thanks for the effort.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1989186475
@geido Ephemeral environment spinning up at http://34.220.148.99:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1989168669
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2014282484
@geido @rusackas i resolved whatever issues pointed out, can you pls review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1763033316
> Sorry for the super late review! Missed this PR. These are not ant design components right? I think single select with predefined list in the dropdown we have in code should be the one used in this for consistency (design and patterns). You can find it in filters for example, and users can also type in some numbers (it just would not accept the new number)
@kasiazjc I have made changes as per your suggestion, i have removed and put select dropdown, now one can also filter based on select dropdown options, now it looks exactly similar to earlier dropdowns, i have checked in chrome, edge, firefox, it is similar in all browsers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1641951637
> @Abhishek-kumar-samsung is this frequency only for a session or persistent?
It is persistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1936327179
@Abhishek-kumar-samsung would you please pull master in your branch and push back?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1521969620
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +165,79 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ {custom_block && (
+ <div
+ style={{
+ display: 'flex',
+ marginTop: '15px',
+ }}
Review Comment:
We prefer not to use inline styles, or hard-coded pixel values. We prefer using Emotion (kind of like Styled Components) and using the Theme's gridUnits. There are some style guides available [here](https://github.com/apache/superset/wiki/Emotion-Styling-Guidelines-and-Best-Practices).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1535001895
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -140,16 +256,52 @@ class RefreshIntervalModal extends React.PureComponent<
}
modalFooter={
<>
+ <Button onClick={this.onCancel} buttonSize="small">
+ {t('Cancel')}
+ </Button>
<Button
buttonStyle="primary"
buttonSize="small"
- onClick={this.onSave}
+ onClick={() => {
Review Comment:
Done
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +165,79 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ {custom_block && (
+ <div
+ style={{
+ display: 'flex',
+ marginTop: '15px',
+ }}
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2008650960
> Hi all - please direct me to an issue link if there's a separate place for questions. We recently adopted Superset and I'm looking into setting up refresh schedules for our dashboards. This PR comes close to introducing the feature I think we need, but really something with the flexibility like cron schedule expressions would be ideal. I haven't been able to understand when the refresh interval triggers (i.e. how it's determined when it'll happen) or to specify a time it should kick off.
>
> Effectively I just want to specify when my refresh interval should start and (based off the time window I choose) when it should stop. Can any help me understand this, please?
Currently in this PR, i was implementing refresh intervals based on relative timing. But crons idea would be nice.
If we need to include crons then this PR will extend to some long time, maybe endlessly.
So maybe we can start an SIP with multiple stages of PR,
First stage maybe this one i.e relative time,
and Second stage can be crons.
What say? @rusackas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "coetzeevs (via GitHub)" <gi...@apache.org>.
coetzeevs commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2008054712
Hi all - please direct me to an issue link if there's a separate place for questions. We recently adopted Superset and I'm looking into setting up refresh schedules for our dashboards. This PR comes close to introducing the feature I think we need, but really something with the flexibility like cron schedule expressions would be ideal. I haven't been able to understand when the refresh interval triggers (i.e. how it's determined when it'll happen) or to specify a time it should kick off.
Effectively I just want to specify when my refresh interval should start and (based off the time window I choose) when it should stop. Can any help me understand this, please?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1507880813
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +169,78 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
Review Comment:
What I am trying to say is that you should remove the element rather than hiding it with opacity.
See for example `{showRefreshWarning && (...)}` a few lines below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2072292405
@Abhishek-kumar-samsung it seems that CI is stuck. Please pull latest master and push back to kick CI on a fresh master state. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1601270195
I have made changes as asked
When user will click **CUSTOM** button then box changes so that one can provide custom frequency, and if user clicks **CUSTOM** button again then original window will appear again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1679432395
Hi @kasiazjc
I added dropdowns in minute and seconds as you have suggested.
Instead of select i used input type with datalist.
In datalist i have provided options from 0-59, if user puts some other value and submits then it will display error message.
datalist is looking little different depending on browsers, some previews as below.
**edge browser preview:**
**firefox browser preview:**
**chrome browser preview:**
I was having problem taking screenshot with dropdown, so i took screenshot using phone.
Please check and review @kasiazjc @rusackas
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Abhishek-kumar-samsung commented on pull request #24449: feat: custom refresh frequency
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1704756975
@kasiazjc @rusackas can you pls check the PR, there's been a long time since no comments, so i thought to remind.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Shisir99 commented on pull request #24449: feat: custom refresh frequency
Posted by "Shisir99 (via GitHub)" <gi...@apache.org>.
Shisir99 commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1641925650
@Abhishek-kumar-samsung is this frequency only for a session or persistent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1507886383
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +100,49 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ this.setState({
+ custom_block: value === -1,
+ });
+
+ if (value === -1) {
+ this.setState({
+ custom_hour: 0,
+ custom_min: 0,
+ custom_sec: 0,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (!refreshIntervalOptions.length) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+
+ refresh_options.push({ value: -1, label: 'Custom interval' });
Review Comment:
What I mean is that you are duplicating the code by using the condition `!refreshIntervalOptions.length` and then pushing the custom interval option, but you also do the same when `refreshIntervalOptions.length` is larger than 0.
So just do:
```
refresh_options.push({ value: -1, label: 'Custom interval' });
if (refreshIntervalOptions.length) {
refresh_options.push(
...refreshIntervalOptions.map(option => ({
value: option[0],
label: t(option[1]),
})),
);
}
return refresh_options;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1521967561
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -140,16 +256,52 @@ class RefreshIntervalModal extends React.PureComponent<
}
modalFooter={
<>
+ <Button onClick={this.onCancel} buttonSize="small">
+ {t('Cancel')}
+ </Button>
<Button
buttonStyle="primary"
buttonSize="small"
- onClick={this.onSave}
+ onClick={() => {
Review Comment:
It would be better if handlers called an external function rather than defining the logic inline.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2011341422
> I agree we don't want to grow the scope of this PR. We should merge this first, and then deal with the cron idea (which is a nice idea) as a subsequent PR.
>
> For the cron feature, it seems like a reasonable plan to seek "lazy consensus" on the dev@ list. Then if that starts an argument, we can upgrade the conversation to a SIP and vote it.
Yes sure, i will quickly do the two changes that you had suggested and once merged, we will raise SIP for cron.
Then we can work on it, @coetzeevs you can also work on that if you had implemented something in it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1939237154
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1936991459
> @Abhishek-kumar-samsung would you please pull master in your branch and push back?
Done, now it is updated to latest master branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1936992198
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1846774428
@kasiazjc hours was normal input text only, it can take any numeric value greater than or equal to 0.
Regarding styling of input text, i removed my custom styling and took one classname from other file and put it.(i.e used styling that was already in the code)
Please check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #24449: feat: custom refresh frequency
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1664410824
Hmm... the persistence is an interesting fork in the road! I'd suggest keeping the functionality as-is for this PR (session-based) and maybe we can discuss a follow-up feature on a separate thread to persist the settings as part of the dashboard itself (probably in the Dashboard metadata). I assume the session-based setting could/would override the dashboard-level setting, too. This might warrant a separate little design discussion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] kasiazjc commented on pull request #24449: feat: custom refresh frequency
Posted by "kasiazjc (via GitHub)" <gi...@apache.org>.
kasiazjc commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1667953187
>   
>
> @kasiazjc i implemented the design as you have suggested, Please review.
Thank you so much! The only thing I would suggest is changing minutes + seconds to a searchable select dropdown component, so that the numbers for seconds (59) and minutes (59) are fixed. Could be a weird experience if people would type in for example 1 hour, 72 minutes, 342 seconds, so I think to avoid that selects would help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1763034637
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1353496153
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +91,52 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ const custom_block = document.getElementById('custom_block_view');
Review Comment:
is it possible to control/select this element with a ref instead of directly fetching from the dom?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Shisir99 commented on pull request #24449: feat: custom refresh frequency
Posted by "Shisir99 (via GitHub)" <gi...@apache.org>.
Shisir99 commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1643364826
@Abhishek-kumar-samsung my dashbaord frequency is saving only for a particular session , how can i make it persistent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #24449: feat: custom refresh frequency
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1599244363
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24449](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d4497e6) into [master](https://app.codecov.io/gh/apache/superset/commit/8bd827679116204aa523c3dd0487104d03ab7376?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (8bd8276) will **increase** coverage by `0.91%`.
> The diff coverage is `90.66%`.
> :exclamation: Current head d4497e6 differs from pull request most recent head 6585324. Consider uploading reports for the commit 6585324 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24449 +/- ##
==========================================
+ Coverage 67.92% 68.83% +0.91%
==========================================
Files 1918 1901 -17
Lines 73882 73988 +106
Branches 8054 8126 +72
==========================================
+ Hits 50183 50931 +748
+ Misses 21646 20946 -700
- Partials 2053 2111 +58
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| javascript | `55.62% <90.66%> (+1.72%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ackages/superset-ui-chart-controls/src/fixtures.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL2ZpeHR1cmVzLnRz) | `100.00% <ø> (ø)` | |
| [...chart-controls/src/shared-controls/dndControls.tsx](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `58.33% <ø> (ø)` | |
| [...d/packages/superset-ui-chart-controls/src/types.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | |
| [...i-core/src/color/colorSchemes/sequential/common.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY29sb3IvY29sb3JTY2hlbWVzL3NlcXVlbnRpYWwvY29tbW9uLnRz) | `100.00% <ø> (ø)` | |
| [...s/superset-ui-core/src/components/SafeMarkdown.tsx](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY29tcG9uZW50cy9TYWZlTWFya2Rvd24udHN4) | `85.71% <0.00%> (+19.04%)` | :arrow_up: |
| [.../superset-ui-core/src/models/ExtensibleFunction.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvbW9kZWxzL0V4dGVuc2libGVGdW5jdGlvbi50cw==) | `100.00% <ø> (ø)` | |
| [...ges/superset-ui-core/src/query/buildQueryObject.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvYnVpbGRRdWVyeU9iamVjdC50cw==) | `100.00% <ø> (ø)` | |
| [...packages/superset-ui-core/src/query/types/Query.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUXVlcnkudHM=) | `100.00% <ø> (ø)` | |
| [...set-ui-core/src/ui-overrides/ExtensionsRegistry.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdWktb3ZlcnJpZGVzL0V4dGVuc2lvbnNSZWdpc3RyeS50cw==) | `100.00% <ø> (ø)` | |
| [...ackages/superset-ui-core/src/utils/featureFlags.ts](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvdXRpbHMvZmVhdHVyZUZsYWdzLnRz) | `100.00% <ø> (ø)` | |
| ... and [16 more](https://app.codecov.io/gh/apache/superset/pull/24449?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [536 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24449/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #24449: feat: custom refresh frequency
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-1629110149
Sorry for the churn and delays on this, but I think the main issue is UI design, not the technical implementation.
My own opinion:
• It would be nice if it had the Select menu (including the Custom option, rather than a button for Custom)
• If (and only if) the Custom option is selected from the Select menu, would the additional fields appear (or disappear)
• The Select menu would remain present... if you select something other than Custom, the form elements are removed, and the values ignored.
As a side note, I feel like the Submit button should be on the right, and the cancel button on the left. I'm not sure if we have a solid documented pattern around this, though (cc. @kasiazjc)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2046038294
After some quick sanity tests on the ephemeral, this is looking pretty good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1502947003
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +100,49 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ this.setState({
+ custom_block: value === -1,
+ });
+
+ if (value === -1) {
+ this.setState({
+ custom_hour: 0,
+ custom_min: 0,
+ custom_sec: 0,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (!refreshIntervalOptions.length) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+
+ refresh_options.push({ value: -1, label: 'Custom interval' });
Review Comment:
The label needs to be localized. Also, aren't you adding this two times as you are adding it when the above condition triggers too?
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -111,17 +169,78 @@ class RefreshIntervalModal extends React.PureComponent<
modalTitle={t('Refresh interval')}
modalBody={
<div>
- <FormLabel>{t('Refresh frequency')}</FormLabel>
- <Select
- ariaLabel={t('Refresh interval')}
- options={refreshIntervalOptions.map(option => ({
- value: option[0],
- label: t(option[1]),
- }))}
- value={refreshFrequency}
- onChange={this.handleFrequencyChange}
- sortComparator={propertyComparator('value')}
- />
+ <div id="refresh_from_dropdown">
+ <FormLabel>
+ <b>{t('Refresh frequency')}</b>
+ </FormLabel>
+ <Select
+ ariaLabel={t('Refresh interval')}
+ options={this.createIntervalOptions(refreshIntervalOptions)}
+ value={refreshFrequency}
+ onChange={this.handleFrequencyChange}
+ sortComparator={propertyComparator('value')}
+ />
+ </div>
+ <div
+ style={{
Review Comment:
Not sure why we are still resorting to style visibility rather than rendering conditionally?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1502945531
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +100,49 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ this.setState({
+ custom_block: value === -1,
+ });
+
+ if (value === -1) {
+ this.setState({
+ custom_hour: 0,
+ custom_min: 0,
+ custom_sec: 0,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (!refreshIntervalOptions.length) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
Review Comment:
I think the label here need to be localized
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "Abhishek-kumar-samsung (via GitHub)" <gi...@apache.org>.
Abhishek-kumar-samsung commented on code in PR #24449:
URL: https://github.com/apache/superset/pull/24449#discussion_r1507774260
##########
superset-frontend/src/dashboard/components/RefreshIntervalModal.tsx:
##########
@@ -91,6 +100,49 @@ class RefreshIntervalModal extends React.PureComponent<
this.setState({
refreshFrequency: value || refreshIntervalOptions[0][0],
});
+
+ this.setState({
+ custom_block: value === -1,
+ });
+
+ if (value === -1) {
+ this.setState({
+ custom_hour: 0,
+ custom_min: 0,
+ custom_sec: 0,
+ });
+ }
+ }
+
+ onSaveValue(value: number) {
+ this.props.onChange(value, this.props.editMode);
+ this.modalRef?.current?.close();
+ this.props.addSuccessToast(t('Refresh interval saved'));
+ }
+
+ createIntervalOptions(refreshIntervalOptions: [number, string][]) {
+ const refresh_options = [];
+ if (!refreshIntervalOptions.length) {
+ refresh_options.push({ value: -1, label: 'Custom interval' });
+ return refresh_options;
+ }
+
+ refresh_options.push({ value: -1, label: 'Custom interval' });
Review Comment:
Ok i will localize it,
no i am not adding twice because from if condition i am returning.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: custom refresh frequency [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #24449:
URL: https://github.com/apache/superset/pull/24449#issuecomment-2075111993
Thanks @Abhishek-kumar-samsung - I/we appreciate you sticking with this and getting it through!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org