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

[#10623] Change design diagrams from Powerpoint to PlantUML #11956

Merged
merged 8 commits into from
Oct 7, 2022

Conversation

tenebrius1
Copy link
Contributor

Part of #10623

Outline of Solution
The diagrams that I have converted are very heavily customized (different colors and line styles were used) and as such there were a lot of inline styling that were done. I am still quite new to PlantUML so I'm not too sure whether these customizations can be done in a better way, but I have represented the original images as best as I can in puml.

For the remaining 2 pptx files (RolesAndTechnologies and workflow), I stand with the previous committer that puml is not sufficient to represent the information presented

@damithc
Copy link
Contributor

damithc commented Sep 11, 2022

General comment: It is OK to keep some diagrams in the current format if the PlantUML version can't be as visually expressive as the current version, especially if the diagram is not expected to change often e.g., Architecture-level diagrams

@fsgmhoward fsgmhoward added the s.ToReview The PR is waiting for review(s) label Sep 12, 2022
@nusoss-bot
Copy link

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

Good effort!
Some suggestions for the high level architecture diagram:

  • Would be good if these 2 arrows can be separated more! Currently looks a bit confusing since the bottom arrow is pointing too close to the top arrow
    image

  • The PlantUML diagram makes it seem like GAE is entirely separate from the rest of the architecture, would be good to shift it directly below Datastore or add an arrow/line to link the two together
    image

Copy link
Contributor

@zhaojj2209 zhaojj2209 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@zhaojj2209 zhaojj2209 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Sep 27, 2022
@nusoss-bot
Copy link

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

@zhaojj2209 zhaojj2209 requested a review from ziqing26 October 6, 2022 05:59
Copy link
Contributor

@ziqing26 ziqing26 left a comment

Choose a reason for hiding this comment

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

LGTM, kindly update the branches to be ready for merging

@ziqing26 ziqing26 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Oct 7, 2022
@zhaojj2209 zhaojj2209 merged commit 7a1e7d3 into TEAMMATES:master Oct 7, 2022
zhengtaoJ pushed a commit to zhengtaoJ/teammates that referenced this pull request Oct 14, 2022
…EAMMATES#11956)

* convert Actors.pptx to puml

* convert highlevelArchitecture.pptx to puml

* migrate IssueLifecycle to puml

* update styling based on comments

Co-authored-by: Zhao Jingjing <54243224+zhaojj2209@users.noreply.github.com>
@wkurniawan07 wkurniawan07 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Dec 25, 2022
@wkurniawan07 wkurniawan07 added this to the V8.20.0 milestone Dec 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants