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

add example wiring diagram #77

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

mrbojangles3
Copy link
Contributor

also experiment with adding line numbers for listings

Signed-off-by: Logan Blyth <[email protected]>
@mrbojangles3 mrbojangles3 requested a review from a team as a code owner March 19, 2025 20:57
Copy link

github-actions bot commented Mar 19, 2025

🚀 Deployed on https://preview-77--hedgehog-docs.netlify.app

@github-actions github-actions bot temporarily deployed to pull request March 19, 2025 20:58 Inactive
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Please find some small suggestions below.

Why is your commit title “initial commit”?

apiVersion: wiring.githedgehog.com/v1beta1
kind: Switch
metadata:
name: ds3000-02
spec:
boot:
serial: ABC123XYZ
serial: ABC123XYZ # (1)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
serial: ABC123XYZ # (1)
serial: ABC123XYZ # or we can also use "mac: 34:AD:61:00:02:03"

And get rid of the (somewhat) confusing footnote?

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 am glad you brought this up. The footnote is something that I though would help, it is good to know that it doesn't. There are other docs that use the footnote style for YAML explanation. Which is what inspired this change.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, the indication itself is useful. Having the footnote right below the YAML sample is OK, but looking at the example you link, I note that:

  • the footnote reference and indicator are the same, both blue circles
  • the marker is clickable, making it easier to find what part of the doc refers to it
  • the note is too big to hold conveniently in a comment on the same line in the YAML sample, which is not really the case here so I'd just as well add the note in the comment, but that's me 🤷

In your case, I would at least make sure to use the same syntax at both locations if you want to keep the footnote, either (1) or 1., but not a mix.

Copy link
Member

Choose a reason for hiding this comment

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

By “confusing” I mostly meant that it was not super easy to find the footnote that it refers to; much of the confusion goes away if you use the same (1) syntax at both locations for example. I did not mean that the content of the note itself was confusing.

@github-actions github-actions bot temporarily deployed to pull request March 20, 2025 13:12 Inactive
@mrbojangles3
Copy link
Contributor Author

The commit message is because I was rushing, I will change it.

Signed-off-by: Pau Capdevila <[email protected]>
@github-actions github-actions bot temporarily deployed to pull request March 21, 2025 09:42 Inactive
@pau-hedgehog
Copy link
Contributor

1. Can also use mac: 34:AD:61:00:02:03 is confusing.

You could take the opportunity to refer to the line number instead, now that you are introducing them

I pushed a commit fixing a small typo.

Probably on another PR:

  • We should refer to the hhfab validate to ensure the wiring YAML is correct
  • Not to mention the visual validation you can do with a diagram! ;)
    (mermaid is supported and can be added in the doc)
  • Maybe I'd take the opportunity to clarify how to define custom passwords. fab.yaml instructs you to:
    password hash use openssl passwd -5`

@mrbojangles3
Copy link
Contributor Author

If we are okay with referring to line numbers I am good with switching to them. There is also the option to have the line numbers start at a given number. So we could have a big listing then break it up in sections to talk about it. Thoughts @pau-hedgehog and @qmonnet

@qmonnet
Copy link
Member

qmonnet commented Mar 21, 2025

If we are okay with referring to line numbers I am good with switching to them

I don't know. Is there a way to reference a specific line an turns that ref into a number under the YAML sample, or do you manually reference the line number, and risks the line number getting outdated if someone inserts more lines into the YAML snippet in the future?

@mrbojangles3
Copy link
Contributor Author

It would be a pretty manual process. We would take the example file and have to manual start the line numbers ourselves. Very easy to get out of sync.

@qmonnet
Copy link
Member

qmonnet commented Mar 21, 2025

I'm still in favour of the footnote (or inline comment) in that case 😅.

@mrbojangles3
Copy link
Contributor Author

What do you think of the look on the material for mkdocs webpage - https://squidfunk.github.io/mkdocs-material/reference/code-blocks/#adding-annotations. It doesn't seem like we are getting the same functionality that is shown on the page.

@qmonnet
Copy link
Member

qmonnet commented Mar 21, 2025

Ooooh so that's what you were trying to do! That's cool, now we just need to understand how to make it work 🙂

@qmonnet
Copy link
Member

qmonnet commented Mar 21, 2025

Try adding a blank line between the YAML snippet and the annotation, just in case?

@pau-hedgehog
Copy link
Contributor

What do you think of the look on the material for mkdocs webpage - https://squidfunk.github.io/mkdocs-material/reference/code-blocks/#adding-annotations. It doesn't seem like we are getting the same functionality that is shown on the page.

image

We may be missing this: https://github.com/pawamoy/mkdocs-pygments ?

@mrbojangles3
Copy link
Contributor Author

mrbojangles3 commented Mar 21, 2025 via email

Signed-off-by: Logan Blyth <[email protected]>
Signed-off-by: Logan Blyth <[email protected]>
@github-actions github-actions bot temporarily deployed to pull request March 21, 2025 19:45 Inactive
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.

3 participants