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 2022/05/12 18:17:37 UTC
[GitHub] [airflow] pierrejeambrun opened a new pull request, #23678: Grid task state filtering with hover and LegendBar style rework
pierrejeambrun opened a new pull request, #23678:
URL: https://github.com/apache/airflow/pull/23678
Follow up of: https://github.com/apache/airflow/pull/23392
This is just a draft for now without proper tests, just to receive some feedbacks.
- LegendRow style has been updated to match the one from the "Graph" view.
- Add a hover effect on the to highlight task state. (The hover effect feels kind of slow, even when leveraging the React.Memo, due to re-rendering hundreds of ''StatusBox" components.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r873197382
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
Yeah, props drilling is appropriate in this case.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r873196265
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
:+1:
I agree, I just did that because we already had the useFilters and it was a conveniant way to pass around this state without having a dedicated Context. (Without pushing this to the url rerendering are not triggered which is the expected behavior)
Fixed. For now I prop drilled a state from the `Main` component, to share this between the `LegendRow` and the `TaskInstances` components.
I didn't want to create a context just for 1 state, but I can change that if preferable.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r871772382
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
We don't want the task state onhover in the url params.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r873196484
##########
airflow/www/static/js/grid/renderTaskRows.jsx:
##########
@@ -56,36 +57,41 @@ const renderTaskRows = ({
const TaskInstances = ({
task, dagRunIds, selectedRunId, onSelect,
-}) => (
- <Flex justifyContent="flex-end">
- {dagRunIds.map((runId) => {
- // Check if an instance exists for the run, or return an empty box
- const instance = task.instances.find((gi) => gi.runId === runId);
- const isSelected = selectedRunId === runId;
- return (
- <Box
- py="4px"
- px={boxPaddingPx}
- className={`js-${runId}`}
- data-selected={isSelected}
- transition="background-color 0.2s"
- key={`${runId}-${task.id}`}
- bg={isSelected && 'blue.100'}
- >
- {instance
- ? (
- <StatusBox
- instance={instance}
- group={task}
- onSelect={onSelect}
- />
- )
- : <Box width={boxSizePx} data-testid="blank-task" />}
- </Box>
- );
- })}
- </Flex>
-);
+}) => {
+ const { filters: { taskState } } = useFilters();
Review Comment:
We are now using a state at higher level. Performances seem better.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r871774810
##########
airflow/www/static/js/grid/renderTaskRows.jsx:
##########
@@ -56,36 +57,41 @@ const renderTaskRows = ({
const TaskInstances = ({
task, dagRunIds, selectedRunId, onSelect,
-}) => (
- <Flex justifyContent="flex-end">
- {dagRunIds.map((runId) => {
- // Check if an instance exists for the run, or return an empty box
- const instance = task.instances.find((gi) => gi.runId === runId);
- const isSelected = selectedRunId === runId;
- return (
- <Box
- py="4px"
- px={boxPaddingPx}
- className={`js-${runId}`}
- data-selected={isSelected}
- transition="background-color 0.2s"
- key={`${runId}-${task.id}`}
- bg={isSelected && 'blue.100'}
- >
- {instance
- ? (
- <StatusBox
- instance={instance}
- group={task}
- onSelect={onSelect}
- />
- )
- : <Box width={boxSizePx} data-testid="blank-task" />}
- </Box>
- );
- })}
- </Flex>
-);
+}) => {
+ const { filters: { taskState } } = useFilters();
Review Comment:
Calling the `useFilters` hook directly here is causing extra rerenders. We should move the hook to the parent component and pass `taskState` as a prop
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1133902509
Everything is working now, thank you @potiuk
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1132221045
I just updated the PR.
Jinja `tojson` function is sorting keys automatically in the returned json.
We can add `flask_app.config['JSON_SORT_KEYS'] = False`, but that is kind of intrusive. Or we can just for this particular use case, declare the `STATE_COLORS` global object directly sorted. This way the sort with the json does not create a change compare to the raw STATE_COLORS obj given to jinja globals. I chose this approach first. The drawback is that it could bring back inconsistencies between the two pages if something add colors to this object breaking the sort of the declared key.
Finally we could also manually sort of both ends this object, but it seems kind of overkill.
Added the `no_status` state with hovering effect as well, and a few tests.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi merged pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
bbovenzi merged PR #23678:
URL: https://github.com/apache/airflow/pull/23678
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1133888018
@pierrejeambrun - I could not see any reason why the build would fail. - I rebased the PR hoping that this was some weird temporary issue. If it fails again, it measn that yarn command fails - this is something you can easily test locally by running the compile script in breeze (as suggested when you enter breeze).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1126979095
@bbovenzi,
Thank you for your feedback and early review. The PR has been updated and I added a few tests 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1127704097
Performance is looking better! If we're going to use a similar ux to the graph view, let's keep it the same order.
<img width="747" alt="Screen Shot 2022-05-16 at 9 50 42 AM" src="https://user-images.githubusercontent.com/4600967/168608059-8393e476-4c5c-46c1-9cfe-e7d9adf2bcd6.png">
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1133888816
Hopefully it was that one that caused it: https://github.com/apache/airflow/pull/23785
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1132413792
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r873196265
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
:+1:
I agree, I just did that because we already had the useFilters and I was comveniant to pass around this state without having a dedicated Context. (Without pushing this to the url rerendering are not triggered which is the expected behavior)
Fixed. For now I prop drilled a state from the `Main` component, to share this between the `LegendRow` and the `TaskInstances` components.
I didn't want to create a context just for 1 state, but I can change that if preferable.
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
:+1:
I agree, I just did that because we already had the useFilters and it was comveniant to pass around this state without having a dedicated Context. (Without pushing this to the url rerendering are not triggered which is the expected behavior)
Fixed. For now I prop drilled a state from the `Main` component, to share this between the `LegendRow` and the `TaskInstances` components.
I didn't want to create a context just for 1 state, but I can change that if preferable.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r873196265
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
:+1:
Fixed. For now I prop drilled a state from the `Main` component, to share this between the `LegendRow` and the `TaskInstances` components.
I didn't want to create a context just for 1 state, but I can change that if preferable.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pierrejeambrun commented on a diff in pull request #23678: Highlight task states by hovering on legend row
Posted by GitBox <gi...@apache.org>.
pierrejeambrun commented on code in PR #23678:
URL: https://github.com/apache/airflow/pull/23678#discussion_r873196265
##########
airflow/www/static/js/grid/utils/useFilters.js:
##########
@@ -44,14 +44,11 @@ const useFilters = () => {
// it is not send to the api for filtering.
const taskState = searchParams.get(TASK_STATE_PARAM);
- const makeOnChangeFn = (paramName, formatFn) => (e) => {
- let { value } = e.target;
- if (formatFn) {
- value = formatFn(value);
- }
+ const makeOnChangeFn = (paramName, formatFn) => (value) => {
Review Comment:
:+1:
For now I prop drilled a state from the `Main` component, to share this between the `LegendRow` and the `TaskInstances` components.
I didn't want to create a context just for 1 state, let me know what you prefer.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] bbovenzi commented on pull request #23678: Grid task state filtering with hover and LegendBar style rework
Posted by GitBox <gi...@apache.org>.
bbovenzi commented on PR #23678:
URL: https://github.com/apache/airflow/pull/23678#issuecomment-1125307423
Sweet! Functionality is great. Let me dig into the performance issues.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org