diagrams icon indicating copy to clipboard operation
diagrams copied to clipboard

Published 20 hours ago • verified publisher icon mingrammer

Add: Twilio

Open zendern opened this issue 4 years ago • 7 comments

Fixes #329

Only did the one icon to fit into current structure of SASS. If someone wants the others specific products maybe adding it as a provider in the future could work. But this gives me what I need for now.

zendern avatar Oct 03 '20 02:10 zendern

I think you made the same mistake here that I did with my Opnsense PR. I was steered to review https://github.com/mingrammer/diagrams/blob/master/CONTRIBUTING.md

Since this adds a new class, do you think a test, or example is warranted?

wolfspyre avatar Oct 04 '20 01:10 wolfspyre

I think you made the same mistake here that I did with my Opnsense PR. I was steered to review https://github.com/mingrammer/diagrams/blob/master/CONTRIBUTING.md

So what mistake are you referring to?? I added the image file, ran autogen per those docs and then check in what was generated.

Since this adds a new class, do you think a test, or example is warranted?

I'll see about working up a test of some form. Honestly there are tons of new images/nodes and it looks to me like the test suite only handles the basic (Cluster, Diagram, Node, Edge) cases.

Maybe a test that validates all "custom nodes" that the auto gen created are rendered as expected?? Some sort of end to end type test to make sure if we change the autogen script in the future the diagrams don't break??

From an example standpoint it probably wouldn't be awfully exciting. Twilio is just a 3rd party SASS/API so its just one line from another node.

zendern avatar Oct 04 '20 01:10 zendern

@zendern can this be merged? I'd love to use it!

AngelAlvarado avatar Dec 03 '20 18:12 AngelAlvarado

@zendern can this be merged? I'd love to use it!

@AngelAlvarado I would like to think so, I do not have merge rights so I cannot just do so. I did have some open questions above as to what mistake I made as I'm still unsure there and I never really figured out testing. But honestly this PR is no different from what I can tell of any other of the new image/node additions.

zendern avatar Dec 03 '20 18:12 zendern

LGTM. But you should resolve the conflicts.

Thanks for the 👍 Conflicts are all cleaned up. This should be ready to go.

zendern avatar Dec 31 '20 13:12 zendern

@mingrammer Any chance of getting this merged since the conflicts got fixed up (~17 months ago)? Would be useful to have in the project!

0xdevalias avatar May 15 '22 01:05 0xdevalias

@mingrammer ping on this one :)

RosterIn avatar Sep 10 '22 08:09 RosterIn

Sorry for the late reply guys.

mingrammer avatar Nov 04 '22 09:11 mingrammer