-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Node as cluster #438
base: master
Are you sure you want to change the base?
Node as cluster #438
Conversation
…com/givanovexpe/diagrams into givanovexpe-feature/add_icons_to_cluster_labels
…0-279-branded-clusters
Node as cluster
@bkmeneguello I suggest to close this in favour of #439 what do you think? did you got a change to go over it? |
I tried this, but I got the error.
Can anyone help? |
Update:
but it's not still displaying the cluster branding. Any solution? One more doubt: |
@bkmeneguello Basically when you do this: from diagrams import Diagram
from diagrams.onprem.network import Nginx
from diagrams.onprem.container import Docker
with Diagram(name="", direction="TB", show=False, filename="aws"):
with Docker("Docker") as container:
with Docker("Docker2 "):
with Docker("Docker3 "):
with Docker("Docker4 "):
with Docker("Docker5 "):
Nginx("Nginx")
container Where the first color is green. If I run the same nested code with actual version I have this: from diagrams import Diagram
from diagrams.onprem.network import Nginx
from diagrams.onprem.container import Docker
with Diagram(name="", direction="TB", show=False, filename="aws"):
with Docker("Docker") as container:
with Docker("Docker2 "):
with Docker("Docker3 "):
with Docker("Docker4 "):
with Docker("Docker5 "):
Nginx("Nginx")
container it render this one: The second problem which is more serious even if there's no tests or example is the support for "graph_attr" in cluster. from diagrams import Diagram, Cluster
from diagrams.onprem.network import Nginx
# from diagrams.onprem.container import Docker
with Diagram(name="", direction="TB", show=False, filename="aws"):
with Cluster("AWS", graph_attr={"pencolor": "#60193C", "bgcolor": "#E587B5"}) as container: # overwrite attributes for the default cluster
Nginx("Nginx")
container
on your PR branch it generate an error:
|
this code is not working on my machine, it throws an error:
|
@seema1711 Your problem is that you use the actual library version which don't have the code of the PR and it's normal that this error raise. |
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.
Here some fix for the problems I found and commented here: #438 (comment)
I didn't find others issue so far.
One last remarks is the examples directory maybe better to move the code in the website.
self.dot.graph_attr["rankdir"] = self._direction | ||
|
||
# Set cluster depth for distinguishing the background color | ||
self.depth = self._parent.depth + 1 |
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.
Here need to be
self.depth = self._parent.depth + 1 if self._parent.depth else 1
To respect the color order.
Check the example in the comment: #438 (comment)
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.
@gabriel-tessier Thank you always for your volunteering. I'll contact you soon.
self.depth = self._parent.depth + 1 | ||
coloridx = self.depth % len(self.__bgcolors) | ||
self.dot.graph_attr["bgcolor"] = self.__bgcolors[coloridx] | ||
|
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.
You need to set the attributs at the end after all the default apply so you can overwrite in your diagrams.
for params, attributs in self._attrs.items():
if params == 'graph_attr':
for k, v in attributs.items():
self.dot.graph_attr[k] = v
self.dot.graph_attr[k] = v | ||
for k, v in self._attrs.items(): | ||
self.dot.graph_attr[k] = v | ||
|
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.
This 2 lines need to be removed: ("The second problem" listed in the comment #438 (comment))
for k, v in self._attrs.items():
self.dot.graph_attr[k] = v
And moved after all the attributes are set also the value of k ad v are not correct chek the code line 378.
Any progress on this one? Would love to have this functionality! |
I'm here. Sorry for the late reply. I'll review this PR ASAP. It could be really good improvements. Thank you :) |
@bkmeneguello Please resolve the conflicts. |
@bkmeneguello Do you have time to resolve the conflicts? We really would like to have this feature. |
Sorry, two years ago I had the time, now I'm not involved anymore with this project. |
I am willing to take this branch and resolve the conflicts if no one minds? I will open this as a new PR. |
This PR contains and supersedes the #407
I'm collaborating with the OP @dan-ash but he seems to be having personal issues to keep this PR up to date.