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

[otns] Update OTNS codelab to OTNS2. #167

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

EskoDijk
Copy link

This updates the OTNS codelab to use the new OTNS2 simulator, which is currently not yet in the OpenThread project Github. The PR for OTNS2 is currently here.

It should be merged/accepted only after we have accepted OTNS2. This could be considered a draft PR, until that time. Some verification / fixes and checking required time may still be needed.

Copy link
Member

@Vyrastas Vyrastas left a comment

Choose a reason for hiding this comment

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

This all looks great, thanks for doing the update! A few comments

@EskoDijk
Copy link
Author

@Vyrastas I've now made an update based on your review. Also added one more paragraph about timestamps correlation.

For merging this update we would probably have to wait until the OTNS2 PR is done (openthread/ot-ns#530) ?

Copy link
Member

@Vyrastas Vyrastas 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 making all the image updates @EskoDijk !

Just some copyedit nits, otherwise looks good to me. And yes we should wait for the other PR to be in before submitting this.

Have you had anyone else try out this updated version of the codelab? I only reviewed from an editorial standpoint, I did not verify that it works as written.

@EskoDijk
Copy link
Author

EskoDijk commented Oct 2, 2024

Thanks, updated with the suggestions now.

Have you had anyone else try out this updated version of the codelab? I only reviewed from an editorial standpoint, I did not verify that it works as written.

No, not yet. I could do a first pass myself to spot any issues, using this rendering. If we know any testing candidates let me know :)

What I notice there is that some screenshots (ones that are wider) need to be enlarged, preferably to fill the entire width of the text-box/content-area in some cases.

As a test, I added a commit using width="95%" (or similar) to set the width relative to the column width , for some pictures. The text is now much better readable. (In the real website, the column is slightly narrower though but comparable.)
So if that looks ok I'll adjust all the images to the right width. For the wider screenshots, they need to fill 100% of column width while for the narrower screenshots it should be less.


<img src="img/d4079cceea0105f0.png" alt="d4079cceea0105f0.png" width="642.12" />
<img src="img/12_7n_del_br.png" alt="Border Router node 9 is deleted" width="642.12" />
Copy link
Member

Choose a reason for hiding this comment

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

I would make the width here consistent with the others, otherwise they are all fine

@Vyrastas
Copy link
Member

Vyrastas commented Oct 3, 2024

As a test, I added a commit using width="95%" (or similar) to set the width relative to the column width , for some pictures. The text is now much better readable. (In the real website, the column is slightly narrower though but comparable.) So if that looks ok I'll adjust all the images to the right width. For the wider screenshots, they need to fill 100% of column width while for the narrower screenshots it should be less.

Everything looks fine on the openthread.io version (which I can't share externally), except for one image which I flagged (smaller width than the others). Thanks for doing the extra work to make it look nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants