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

Upgrade plant uml new theme #202

Merged

Conversation

Eclipse-Dominator
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator commented Jul 27, 2023

Fixes #200

Alternative to #201 where the new default theme is used instead of the old rose theme.

PlantUML(1.2023.2) introduced a change in behavior when rendering
diagrams. This causes some of the diagrams to be rendered incorrectly or
fail to compile. Diagrams are also being rendered in a different theme
due to a change in the default color theme.

Therefore, some of the PlantUML code and images need to be updated to
ensure functionality and consistency.

Let's

  • update PlantUML code to ensure they can be rendered properly
  • update the diagrams to ensure consistency between the PlantUML code
    and the rendered image

@canihasreview
Copy link

canihasreview bot commented Jul 27, 2023

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #202 (aafaef4) into master (a976ec9) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #202   +/-   ##
=========================================
  Coverage     74.03%   74.03%           
  Complexity      423      423           
=========================================
  Files            71       71           
  Lines          1290     1290           
  Branches        127      127           
=========================================
  Hits            955      955           
  Misses          301      301           
  Partials         34       34           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@canihasreview
Copy link

canihasreview bot commented Jul 27, 2023

v1

@Eclipse-Dominator submitted v1 for review.

(📚 Archive)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/202/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@damithc
Copy link
Contributor

damithc commented Jul 27, 2023

@Eclipse-Dominator do you see any diagrams that is not optimal under the default theme (compared to the old theme)?

For the following, I'm not sure why the Storage component appears in a different color.
image

But while we are at it, we can actually color each of those two boxes with a very light shade of the main color we use the same components in the architecture diagram.

@Eclipse-Dominator
Copy link
Contributor Author

Eclipse-Dominator commented Jul 27, 2023

@damithc I believe there is some rendering logic for Packages that contain classes and Packages that doesn't. The Model package above contains a hidden Class but the Storage package doesn't. This causes a change in rendering.

do you see any diagrams that is not optimal under the default theme (compared to the old theme)?

Mainly it would be the UndoRedoState diagrams.
image

The main differences between the 2 themes are:

  • Classes are no longer colored
  • Borders, arrows and lines are black by default (previously red)
  • Change in the color of note

@damithc
Copy link
Contributor

damithc commented Jul 27, 2023

The main differences between the 2 themes are:

  • Classes are no longer colored
  • Borders, arrows and lines are black by default (previously red)
  • Change in the color of note

Personally, I think the old theme is slightly easier to read. But given that using the defaults is less work, I'm fine to switch to the new theme despite the slight loss of readability.

@Eclipse-Dominator
Copy link
Contributor Author

Eclipse-Dominator commented Jul 27, 2023

Personally, I think the old theme is slightly easier to read. But given that using the defaults is less work, I'm fine to switch to the new theme despite the slight loss of readability.

@damithc It will still be the same amount of work as all the formatting the diagram component to the AB3 style still require the student to import style.puml. So there are actually no difference whether we are using the new theme or the old theme

@Eclipse-Dominator Eclipse-Dominator force-pushed the upgrade-plant-uml-new-theme branch 2 times, most recently from e4730c0 to 7aa0bcb Compare July 27, 2023 23:20
@canihasreview
Copy link

canihasreview bot commented Jul 27, 2023

v2

@Eclipse-Dominator submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/202/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Copy link
Contributor

@damithc damithc left a comment

Choose a reason for hiding this comment

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

Looks good.
The following part of the commit message can be shortened as it is mostly stating the obvious:

Therefore, some of the PlantUML code and images need to be updated to
ensure functionality and consistency.

Let's
 * update PlantUML code to ensure they can be rendered properly
 * update the diagrams to ensure consistency between the PlantUML code
   and the rendered image

Something like this?

Let's update the PlantUML diagrams accordingly.

@damithc
Copy link
Contributor

damithc commented Jul 29, 2023

@se-edu/tech-team-level1 for your review ...

Copy link

@dlimyy dlimyy left a comment

Choose a reason for hiding this comment

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

Overall, LGTM! Just one minor suggestion

Copy link

Choose a reason for hiding this comment

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

For the new image, it would be good if the association role filtered can be shifted to the association pointing from ModelManager to Person(similar to the old image) since it is the ModelManager that contains the filtered list of Person.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an invisible class to separate the 2 labels.

@canihasreview
Copy link

canihasreview bot commented Jul 29, 2023

v3

@Eclipse-Dominator submitted v3 for review.

(📚 Archive) (📈 Interdiff between v2 and v3) (📈 Range-Diff between v2 and v3)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/202/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

Copy link

@SPWwj SPWwj left a comment

Choose a reason for hiding this comment

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

The PR looks good, as it standardizes the hexadecimal color from 5 to 6 digits. However, due to this change, the style of some diagrams may not look as good as before. It would be better if we could adjust the style to make the diagrams easier to read.

  1. I've noticed that previously all labels were bold, but in the new version, they are not. It might be beneficial to restore the bold formatting for better readability.

  2. The default color of the lines is not the same as in the previous version, but I think this is acceptable as it doesn't significantly impact the understanding of the diagrams.

Copy link

Choose a reason for hiding this comment

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

The style of this diagram is slightly different. Can we adjust the CSS to make the text and nodes stand out, making them easier to read?

Copy link

Choose a reason for hiding this comment

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

The original diagram had better contrast and was easier to read. Can we adjust the CSS to make it similar to the original diagram?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to directly make font thicker but I compensated by bumping the font size slightly to make the font appear clearer.

@Eclipse-Dominator Eclipse-Dominator force-pushed the upgrade-plant-uml-new-theme branch 5 times, most recently from 8eab26c to 8277ee0 Compare July 30, 2023 02:07
PlantUML(1.2023.2) introduced a change in behavior when rendering
diagrams. This causes some of the diagrams to be rendered incorrectly or
fail to compile. Diagrams are also being rendered in a different theme
due to a change in the default color theme.

Let's update the PlantUML diagrams accordingly.
PlantUML diagrams in doc/diagrams/plantuml are not used in the project.
The rendered image of this code are being used in SE-EDU's plantUml
tutorial (https://se-education.org/guides/tutorials/plantUml.html).

As these images are not part of AB3, let's remove these diagrams.
@canihasreview
Copy link

canihasreview bot commented Jul 30, 2023

v4

@Eclipse-Dominator submitted v4 for review.

(📚 Archive) (📈 Interdiff between v3 and v4) (📈 Range-Diff between v3 and v4)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/202/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@damithc damithc merged commit 38922aa into se-edu:master Aug 4, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade UML diagrams to the new version of PlantUML
4 participants