-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Update architecture-diagram.png #829
Conversation
this looks clean!! thanks for tackling the issues too! |
🚲 PR staged at http://35.226.58.146 |
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 LGTM, but maybe wait for Ahrar's feedback/thoughts!
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 looks much, much easier to read, thank you for this!
Small nit: could you make the arrow from frontend->ad start at the bottom of frontend, and not from the side?
At first glance it looks like there's an arrow from loadgen->ad which took me a while to unsee.
Similarly, could we have the arrows to payment / email also start from the bottom, for consistency?
🚲 PR staged at http://35.226.58.146 |
@ckim328 Thanks for the quick review! @bourgeoisor Good feedback! Thank you! Arrows fixed. ✅ |
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.
Wow, what a great improvement to the diagram! As the first technical artifact that informs the user of the app's architecture, it is very important to get this right. This revision is much easier to consume at a glance. Great job, @NimJay!! 👏
I have some thoughts, but none of them are blocking:
- To be precise, the User/loadgenerator can communicate over both HTTP and HTTPS. In that case, the technically correct notation would be HTTP(S)
- I would suggest using the a darker shade of grey/black on the borders Service nodes (keep it consistent with the font color, too)
- Should be "Redis cache"
- The arrows from frontend to User/loadgenerator should be bi-directional (..right?)
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 addressing my comment, this looks amazing!
Thank you, @ahrarmonsur, for the second round of feedback!
Good suggestion.
Good suggestion, but when I tried it out, it seems like the rectangles steal attention away from the text (see slide 2 vs slide 1).
Fixed. It's now "cache" — not "Cache".
The Merging. |
🚲 PR staged at http://35.226.58.146 |
* Update architecture-diagram.png * Fix arrows in architecture-diagram.png * Use 'cache' in architecture-diagram.png
Change Summary
docs/img/architecture-diagram.png
.Fixes
1. Typo
2. Public Access to Source Files
3. Public Access to Source Files
@ahrarmonsur's Feedback
frontend
service the focal point. I've bolded "frontend" and added a maroon/pink-ish background.adservice
).