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

add selector for python models, creating default node color in DAG visualization #324

Conversation

paulbenschmidt
Copy link
Contributor

@paulbenschmidt paulbenschmidt commented Oct 17, 2022

resolves dbt-labs/dbt-core#5880

Description

Changes default color assignment of Python models in lineage graph so that they're easily distinguishable from SQL models. After scanning the current colors, I figured that some type of yellow would be the safest bet; I'm including a few variations below...

#e3ad06 (proposed) - it looks a little like mustard, but I wanted it to be dark enough for the characters to contrast
Screen Shot 2022-10-17 at 4 59 18 PM

Checklist

@cla-bot
Copy link

cla-bot bot commented Oct 17, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Paul Schmidt.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

2 similar comments
@cla-bot
Copy link

cla-bot bot commented Oct 17, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Paul Schmidt.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@cla-bot
Copy link

cla-bot bot commented Oct 17, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Paul Schmidt.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@emmyoop emmyoop added the Hackathon Coalesce Hackathon Submissions label Oct 17, 2022
@emmyoop emmyoop self-requested a review October 17, 2022 21:51
Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

@paulbenschmidt thanks for this contribution! I tested it out and everything looks great and works just as expected.

Is your email associated with your github handle? I checked our cla database and your handle is there as expected.

You'll also need to add a changelog entry. Seems like our docs are missing that piece of info! You can use the instructions here.

@jtcohen6 or @lostmygithubaccount can I get your input on the default node color, please?

@cla-bot
Copy link

cla-bot bot commented Oct 17, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Paul Schmidt.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

1 similar comment
@cla-bot
Copy link

cla-bot bot commented Oct 17, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Paul Schmidt.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@paulbenschmidt
Copy link
Contributor Author

paulbenschmidt commented Oct 17, 2022

@paulbenschmidt thanks for this contribution! I tested it out and everything looks great and works just as expected.

You'll also need to add a changelog entry. Seems like our docs are missing that piece of info! You can use the instructions here.

@jtcohen6 or @lostmygithubaccount can I get your input on the default node color, please?

Thank you, @emmyoop! I revised the color so that it was darker and contrasted more with the white text. I also added a changlelog entry!

Is your email associated with your github handle?

It is! I'm not sure why I can't get the error to disappear...

@cla-bot
Copy link

cla-bot bot commented Oct 17, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Paul Schmidt.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email [email protected]
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@emmyoop
Copy link
Member

emmyoop commented Oct 18, 2022

@paulbenschmidt it looks like one of your commits was associated with a different email. You can either squash all your commits down to a single commit or you can create a new fork and move all your changes there with a new PR. That should resolve the CLA issue.

@paulbenschmidt paulbenschmidt force-pushed the ct_1205_ps_change_python_model_default_color branch from 3051089 to 2543984 Compare October 18, 2022 22:32
@cla-bot cla-bot bot added the cla:yes label Oct 18, 2022
@paulbenschmidt
Copy link
Contributor Author

@emmyoop - Thanks for the help! Squashing the commits did it! If you want me to start fresh in a new branch so that it's a bit cleaner, let me know!

@emmyoop
Copy link
Member

emmyoop commented Oct 19, 2022

@paulbenschmidt that's great news! We squash all the commits down when we merge anyways so this is a fine solution. Once I get some feedback from @lostmygithubaccount or @jtcohen6 on the default color we will be good to merge!

@lostmygithubaccount
Copy link

thanks for this contribution @paulbenschmidt!

I think keeping in line with our blue-ish color scheme in the DAG, it'd be better to make this a more subtle blue variant by default. would you (or @emmyoop?) mind updating to another color? perhaps a "SlateBlue"? hex code #6A5ACD

I haven't actually had a chance to see it myself. post-Coalesce, I think we'll consult with our design team on a default color scheme

@emmyoop
Copy link
Member

emmyoop commented Oct 20, 2022

I've created #325 to track revisiting the color scheme as a whole.

@paulbenschmidt, once you get the color updated to #6A5ACD this will be good to go!

@paulbenschmidt
Copy link
Contributor Author

@emmyoop - Done! Example screenshot included below...
Screen Shot 2022-10-20 at 1 43 57 PM

Copy link
Member

@emmyoop emmyoop left a comment

Choose a reason for hiding this comment

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

Thanks so much @paulbenschmidt for this contribution! Can't wait to see your next one 😃

@emmyoop emmyoop merged commit a004899 into dbt-labs:main Oct 20, 2022
@lostmygithubaccount lostmygithubaccount changed the title add selector for python models add selector for python models, creating default node color in DAG visualization Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Hackathon Coalesce Hackathon Submissions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1205] [Feature] default dbt-sql and dbt-py models to different colors in the DAG viz
3 participants