Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UI: Selection on graph with edges. Status for jobs/datasets #2384

Merged
merged 9 commits into from
Feb 9, 2023

Conversation

tito12
Copy link
Contributor

@tito12 tito12 commented Jan 28, 2023

Problem

According to the issue we are not showing status for jobs and datasets in way we need - based on last quality facets on for datasets and last 14 runs for jobs. This logic should be update, to show this info in correct way.
Also path of selected node and connected to it - is not highligted.
We need to show status of runs and quality facets also in tables with those data, based on the same logic with last 14 records.

Closes: #2169

Solution

This PR provide those changes

image

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've updated the CHANGELOG.md with details about your change under the "Unreleased" section (if relevant, depending on the change, this may not be necessary)
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
  • You've included a header in any source code files (if relevant)

Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
@tito12 tito12 changed the title Selection on graph with edges. Status for jobs/datasets UI: Selection on graph with edges. Status for jobs/datasets Jan 28, 2023
Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
@wslulciuc wslulciuc requested a review from phixMe January 30, 2023 21:39
docker/metadata.json Outdated Show resolved Hide resolved
docker/metadata.json Show resolved Hide resolved
@wslulciuc
Copy link
Member

wslulciuc commented Jan 30, 2023

@tito12, did you link the correct issue in your PR description? #2167 is closed and references the soft delete functionality (which has already been added). Anyways, highlighting the lineage path in the graph looks really cool!

web/package.json Outdated
"eslint-plugin-react": "^7.27.0",
"eslint-plugin-sort-imports-es6-autofix": "^0.6.0",
"file-loader": "^6.2.0",
"html-webpack-plugin": "^5.5.0",
"identity-obj-proxy": "^3.0.0",
"jest": "27.3.1",
"prettier": "1.18.2",
"prettier": "^1.19.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're bumping versions here for things not related to this workflow. I'm happy to have a PR that only updates these versions, but I think this could potentially cause unrelated issues that are hard to debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated version for this package to pass tests for new ecmascript features as "optional chaining", because current doesn't support it and throw errors. Do you mean that I can create separate PR for it and it's be okay? Or if I understood wrong, what do you think will be better solution to resolve it? Because it's make code more clean and readable. Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a seperate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, did it, please review #2393

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also reverted changes with versions for this PR.

web/src/helpers/nodes.ts Outdated Show resolved Hide resolved
web/src/helpers/nodes.ts Outdated Show resolved Hide resolved
@tito12
Copy link
Contributor Author

tito12 commented Feb 1, 2023

@tito12, did you link the correct issue in your PR description? #2167 is closed and references the soft delete functionality (which has already been added). Anyways, highlighting the lineage path in the graph looks really cool!

You are right. Added wrong ID by mistake. Already updated to correct

Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
@tito12
Copy link
Contributor Author

tito12 commented Feb 2, 2023

This PR not contain changes for status of jobs on graph, because it's not possible to get all runs with polling each one separately, only for selected job. It's need changes from API side to be able to do that.

web/src/helpers/nodes.ts Outdated Show resolved Hide resolved
Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
Copy link
Member

@phixMe phixMe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, 🐑 it!

@wslulciuc
Copy link
Member

wslulciuc commented Feb 7, 2023

It's need changes from API side to be able to do that.

@tito12: what API changes would be needed? And would you mind opening an issue to continue the dicussion?

Signed-off-by: tito12 <vladyslav.sedenko@gmail.com>
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #2384 (5e3ffcf) into main (9e0c84b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #2384   +/-   ##
=========================================
  Coverage     77.11%   77.11%           
  Complexity     1234     1234           
=========================================
  Files           228      228           
  Lines          5572     5572           
  Branches        447      447           
=========================================
  Hits           4297     4297           
  Misses          775      775           
  Partials        500      500           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@wslulciuc wslulciuc merged commit f9465bf into main Feb 9, 2023
@wslulciuc wslulciuc deleted the web/graph-display branch February 9, 2023 14:39
@howardyoo
Copy link
Collaborator

howardyoo commented Feb 9, 2023

I've checked the view and it looks great! One comment: the color white to highlight the up and down stream does not seem to stand out too much. Perhaps we could use other colors such as #71ddbf to highlight them, as well as the selected node (job or dataset). Not sure if this little change of color should require a separate PR, but would like to see if @tito12 is willing to perhaps patch it, if possible.

@tito12
Copy link
Contributor Author

tito12 commented Feb 21, 2023

I've checked the view and it looks great! One comment: the color white to highlight the up and down stream does not seem to stand out too much. Perhaps we could use other colors such as #71ddbf to highlight them, as well as the selected node (job or dataset). Not sure if this little change of color should require a separate PR, but would like to see if @tito12 is willing to perhaps patch it, if possible.

Thanks for feedback and sorry for long answer, current PR is closed and merged, but it's not a problem to create another one for it. Do you mean also make this color (kind of green) for selected node with path?

@howardyoo
Copy link
Collaborator

I've checked the view and it looks great! One comment: the color white to highlight the up and down stream does not seem to stand out too much. Perhaps we could use other colors such as #71ddbf to highlight them, as well as the selected node (job or dataset). Not sure if this little change of color should require a separate PR, but would like to see if @tito12 is willing to perhaps patch it, if possible.

Thanks for feedback and sorry for long answer, current PR is closed and merged, but it's not a problem to create another one for it. Do you mean also make this color (kind of green) for selected node with path?

Hi @tito12 , yes. I think having the selected node to be colored in 'green' would be great. It doesn't have to be exactly same color as the lines (perhaps the node could be a little darker?) but that would stand out really well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: Lineage Graph display mode enhancements
4 participants