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

[#53] Add section for reference frame #62

Closed
wants to merge 7 commits into from

Conversation

mfjkri
Copy link
Contributor

@mfjkri mfjkri commented Jul 22, 2024

Fixes #51

Copy link
Contributor

@NeoHW NeoHW left a comment

Choose a reason for hiding this comment

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

good changes overall, might have to standardise some things

tutorials/plantUml.md Show resolved Hide resolved
<tr class="odd">
<td>
<pre>
ref over MSLogic, TextUi : get minefield appearance
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment would be helping to tell students that this is for the reference frame only.
OR
Moreover, it might be better to simplify the image. It could focus on the section between :TextUi and :MsLogic. Then, include the entire code within the Source block, similar to how the full code for the overall image in above sections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion!

I have went ahead and simplified the diagrams as much I could. I have not simplified it fully as I was concerned that it could undermine the example's relevance. If the diagrams are too simplified, it might be harder to see and connect the usage of reference frames in an actual use case.

Could you suggest any specific areas that could be removed, or do you think the current diagrams are sufficient?

@mfjkri mfjkri changed the title Add section for reference frame [#53] Add section for reference frame Jul 22, 2024
Copy link

@gongg21 gongg21 left a comment

Choose a reason for hiding this comment

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

Overall, simple and clear implementation of the tip!

tutorials/plantUml.md Show resolved Hide resolved
tutorials/plantUml.md Outdated Show resolved Hide resolved
tutorials/plantUml.md Outdated Show resolved Hide resolved
tutorials/plantUml.md Outdated Show resolved Hide resolved
@damithc
Copy link
Contributor

damithc commented Aug 8, 2024

@mfjkri Let's wrap this up.

@damithc
Copy link
Contributor

damithc commented Aug 20, 2024

@mfjkri Let me know when this is ready for review again.

@mfjkri
Copy link
Contributor Author

mfjkri commented Aug 21, 2024

@damithc Yup its ready for review again!

Copy link
Contributor

@baskargopinath baskargopinath left a comment

Choose a reason for hiding this comment

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

thanks for the changes. LGTM!

damithc pushed a commit that referenced this pull request Sep 8, 2024
Based on #62

author: @mfjkri
(cherry picked from commit e6c8b6da45d5d7b1add45b5490a92ddebeb7f17e)
damithc pushed a commit that referenced this pull request Sep 8, 2024
@damithc
Copy link
Contributor

damithc commented Sep 8, 2024

Thanks for this PR, @mfjkri
I've incorporated your changes via d6721a9

@damithc damithc closed this Sep 8, 2024
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.

PlantUML guide: Add tip about
5 participants