You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/10/14 21:21:06 UTC
[GitHub] [airflow] ryanahamilton opened a new pull request #11534: Feature: Auto-refresh Graph view chart
ryanahamilton opened a new pull request #11534:
URL: https://github.com/apache/airflow/pull/11534
- Ties in to (and keeps) the existing manual refresh to trigger automatically every 3 seconds
- Auto-refresh is turned on by default. If turned "off", that preference is stored in `localStorage` and will be "off" for any subsequent visits to the Graph view of this and any other DAG.
- Added a new "loading dots" visual feedback to indicate to the user when an update is occurring. This replaced the previous spinner GIF.
**In action:**
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #11534:
URL: https://github.com/apache/airflow/pull/11534
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#issuecomment-708669804
This is awesome 🚀
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#discussion_r505507886
##########
File path: airflow/www/templates/airflow/graph.html
##########
@@ -453,27 +467,56 @@
return false
}
- function initRefreshButton() {
- d3.select("#refresh_button").on("click", () => {
- $("#loading").css("display", "block");
- $("div#svg_container").css("opacity", "0.2");
- $.get(getTaskInstanceURL)
- .done(
- (tis) => {
- task_instances = JSON.parse(tis)
- updateNodesStates(task_instances);
- $("#loading").hide();
- $("div#svg_container").css("opacity", "1");
- $('#error').hide();
- }
- ).fail((_, textStatus, err) => {
- $('#error_msg').html(`${textStatus}: ${err}`);
- $('#error').show();
- $('#loading').hide();
- $('#chart_section').hide(1000);
- $('#datatable_section').hide(1000);
- });
- });
+ function handleRefresh() {
+ $('#loading-dots').css('display', 'inline-block');
+ $('.graph-svg-wrap').fadeTo(400, 0.5);
Review comment:
Yeah, I think you're right about that—removing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#discussion_r505529482
##########
File path: airflow/www/templates/airflow/graph.html
##########
@@ -453,27 +467,56 @@
return false
}
- function initRefreshButton() {
- d3.select("#refresh_button").on("click", () => {
- $("#loading").css("display", "block");
- $("div#svg_container").css("opacity", "0.2");
- $.get(getTaskInstanceURL)
- .done(
- (tis) => {
- task_instances = JSON.parse(tis)
- updateNodesStates(task_instances);
- $("#loading").hide();
- $("div#svg_container").css("opacity", "1");
- $('#error').hide();
- }
- ).fail((_, textStatus, err) => {
- $('#error_msg').html(`${textStatus}: ${err}`);
- $('#error').show();
- $('#loading').hide();
- $('#chart_section').hide(1000);
- $('#datatable_section').hide(1000);
- });
- });
+ function handleRefresh() {
+ $('#loading-dots').css('display', 'inline-block');
+ $('.graph-svg-wrap').fadeTo(400, 0.5);
+ $.get(getTaskInstanceURL)
+ .done(
+ (tis) => {
Review comment:
IE support is thankfully not been a concern right now, and since it already exists, yeah it can stay.
(Probably worth extracting the inline JS to a separate script in a separate PR as you say)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#issuecomment-708677670
[The Workflow run](https://github.com/apache/airflow/actions/runs/307378242) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#discussion_r505336754
##########
File path: airflow/www/templates/airflow/graph.html
##########
@@ -453,27 +467,56 @@
return false
}
- function initRefreshButton() {
- d3.select("#refresh_button").on("click", () => {
- $("#loading").css("display", "block");
- $("div#svg_container").css("opacity", "0.2");
- $.get(getTaskInstanceURL)
- .done(
- (tis) => {
- task_instances = JSON.parse(tis)
- updateNodesStates(task_instances);
- $("#loading").hide();
- $("div#svg_container").css("opacity", "1");
- $('#error').hide();
- }
- ).fail((_, textStatus, err) => {
- $('#error_msg').html(`${textStatus}: ${err}`);
- $('#error').show();
- $('#loading').hide();
- $('#chart_section').hide(1000);
- $('#datatable_section').hide(1000);
- });
- });
+ function handleRefresh() {
+ $('#loading-dots').css('display', 'inline-block');
+ $('.graph-svg-wrap').fadeTo(400, 0.5);
+ $.get(getTaskInstanceURL)
+ .done(
+ (tis) => {
+ task_instances = JSON.parse(tis)
+ updateNodesStates(task_instances);
+ setTimeout(function() { $('#loading-dots').hide(); }, 500);
+ $('.graph-svg-wrap').fadeTo(400, 1);
+ $('#error').hide();
+ }
+ ).fail((_, textStatus, err) => {
+ $('#error_msg').html(`${textStatus}: ${err}`);
Review comment:
I know you didn't change this, but I think this should be `.text()`, not `.html()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#discussion_r505335147
##########
File path: airflow/www/templates/airflow/graph.html
##########
@@ -453,27 +467,56 @@
return false
}
- function initRefreshButton() {
- d3.select("#refresh_button").on("click", () => {
- $("#loading").css("display", "block");
- $("div#svg_container").css("opacity", "0.2");
- $.get(getTaskInstanceURL)
- .done(
- (tis) => {
- task_instances = JSON.parse(tis)
- updateNodesStates(task_instances);
- $("#loading").hide();
- $("div#svg_container").css("opacity", "1");
- $('#error').hide();
- }
- ).fail((_, textStatus, err) => {
- $('#error_msg').html(`${textStatus}: ${err}`);
- $('#error').show();
- $('#loading').hide();
- $('#chart_section').hide(1000);
- $('#datatable_section').hide(1000);
- });
- });
+ function handleRefresh() {
+ $('#loading-dots').css('display', 'inline-block');
+ $('.graph-svg-wrap').fadeTo(400, 0.5);
Review comment:
I'm not sure we want this fade to happen every few 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#issuecomment-708690397
@ashb I refactored it a bit in this PR… Previously the containing gray box had its opacity reduced and it was a bit janky when paired with the GIF overlay. I updated it so that the SVG element within the gray box had its opacity reduced more subtly when updating (it looks smoother IRL than the PR GIF).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton edited a comment on pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ryanahamilton edited a comment on pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#issuecomment-708690397
@ashb I refactored it a bit in this PR… Previously the containing gray box had its opacity reduced and it was a bit janky when paired with the GIF overlay. I updated it so that just the SVG element within the gray box had its opacity reduced more subtly when updating (it looks smoother IRL than the PR GIF).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#discussion_r505336116
##########
File path: airflow/www/templates/airflow/graph.html
##########
@@ -453,27 +467,56 @@
return false
}
- function initRefreshButton() {
- d3.select("#refresh_button").on("click", () => {
- $("#loading").css("display", "block");
- $("div#svg_container").css("opacity", "0.2");
- $.get(getTaskInstanceURL)
- .done(
- (tis) => {
- task_instances = JSON.parse(tis)
- updateNodesStates(task_instances);
- $("#loading").hide();
- $("div#svg_container").css("opacity", "1");
- $('#error').hide();
- }
- ).fail((_, textStatus, err) => {
- $('#error_msg').html(`${textStatus}: ${err}`);
- $('#error').show();
- $('#loading').hide();
- $('#chart_section').hide(1000);
- $('#datatable_section').hide(1000);
- });
- });
+ function handleRefresh() {
+ $('#loading-dots').css('display', 'inline-block');
+ $('.graph-svg-wrap').fadeTo(400, 0.5);
+ $.get(getTaskInstanceURL)
+ .done(
+ (tis) => {
Review comment:
Modern browsers all support arrow-functions natively right? I.e. we don't have to worry about this not be transpiled because it's in a template and not processed through webpack.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ryanahamilton commented on a change in pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#discussion_r505506462
##########
File path: airflow/www/templates/airflow/graph.html
##########
@@ -453,27 +467,56 @@
return false
}
- function initRefreshButton() {
- d3.select("#refresh_button").on("click", () => {
- $("#loading").css("display", "block");
- $("div#svg_container").css("opacity", "0.2");
- $.get(getTaskInstanceURL)
- .done(
- (tis) => {
- task_instances = JSON.parse(tis)
- updateNodesStates(task_instances);
- $("#loading").hide();
- $("div#svg_container").css("opacity", "1");
- $('#error').hide();
- }
- ).fail((_, textStatus, err) => {
- $('#error_msg').html(`${textStatus}: ${err}`);
- $('#error').show();
- $('#loading').hide();
- $('#chart_section').hide(1000);
- $('#datatable_section').hide(1000);
- });
- });
+ function handleRefresh() {
+ $('#loading-dots').css('display', 'inline-block');
+ $('.graph-svg-wrap').fadeTo(400, 0.5);
+ $.get(getTaskInstanceURL)
+ .done(
+ (tis) => {
Review comment:
Well [up to IE 11 does not](https://caniuse.com/arrow-functions). This and the 7 other instances in this file were pre-existing. There appear to be 12 instances total throughout all of the HTML template files. Maybe worth a follow-up PR to refactor all of them at once?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on pull request #11534: Feature: Auto-refresh Graph view chart
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #11534:
URL: https://github.com/apache/airflow/pull/11534#issuecomment-708672295
Is it worth removing the opacity overlay when refreshing too?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org