-
Notifications
You must be signed in to change notification settings - Fork 226
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 option to add colors to connector pins #141
Conversation
825ea4d
to
127c720
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry for picking on all these small issues, but when reviewing, I want to do a thorough job, and include all issues I feel can be improved, but I respect that others might disagree on my issues.
Your nitpicking is excellent and much appreciated. The point of doing code review is to produce cleaner, easier to understand code. I will include most of your suggestions and reply to them if anything is unclear. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4093a6a
to
e3fb39f
Compare
Playing around a bit more. The current output generates a small black outline around each pin color box. 🎉 If the connector node is stretched due to long parameter strings in the top row, the pin color cells end up looking rather wide, since GraphViz autoscales all columns. The problem is worsened because two cells/columns are needed for the pin color: one for the color string, one for the little box. GraphViz does not allow mixing strings and nested tables within a single cell. Using |
I played with this a while ago. The problem is, that there is no way mixing fixed sized and auto-sized colums so all colums are stretched evenly. Making the content of a cell fixed size (like your 2nd image) does not help at all. I have not found a way around that so eventually I gave up. |
TBH, I'm quite happy with the result so far (first picture), and wouldn't mind leaving it like it is. |
8166a63
to
4406816
Compare
4406816
to
347235a
Compare
347235a
to
e8bfbd3
Compare
e8bfbd3
to
08ade98
Compare
@kvid I tried to request a review from you, but it seems that isn't working. So please have a look at this and let me know your thoughts :) As mentioned above, I'm sure the appearance can be fine-tuned, but I want to get the feature included to get some feedback. |
08ade98
to
711704d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using itertools.zip_longest
as you have already considered, you can not only simplify your new code, but also some of the old Connector.__post_init__()
code.
I also miss a demonstration of this feature in one of the examples.
Thanks. I have implemented your suggestions; I appreciate the simplification of the code. Regarding examples: I will need to add/change a lot of examples once #186 is implemented, therefore I will wait until then. I hope to create some more realistic examples to showcase the individual features better, not just randomly add it to one of the current ones. Therefore, I will go ahead and merge this soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for accepting my previous suggestions.
This time, I only have a few not-so-important suggestions...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for accepting most of my suggestions again. As you want to keep the non-zero pincount
requirement, then I suggest simplifying this test and also allow pincolors
list length to set default pincount
. Then all the 3 pin attribute lists influence the pincount
equally.
Co-authored-by: kvid <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for accepting my last suggestion as well. I think this PR is ready now. 😃
Closes #53.
Based on
feature/gv-html-refactor
branch.pincolors
list attribute to connectors.pincolors
is used, needs to be the same length aspins
,pinlabels
connector.color
mark on purpose__
as placeholder for pins with no color__
,--
,XX
and anything else may be used.Example: