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 troubleshooting section to variables doc #1417

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

Conversation

michelleN
Copy link
Member

Content must go through a pre-merge checklist.

Pre-Merge Content Checklist

This documentation has been checked to ensure that:

  • The title, template, and date are all set
  • Does this PR have a new menu item (anywhere in templates/*.hbs files) that points to a document .md that is set to publish in the future? If so please only publish the .md and .hbs changes in real-time (otherwise there will be a menu item pointing to a .md file that does not exist)
  • File does not use CRLF, but uses plain LF (hint: use cat -ve <filename> | grep $'\r' | wc -l and expect 0 as a result)
  • Has passed bart check
  • Has been manually tested by running in Spin/Bartholomew (hint: use PREVIEW_MODE=1 and run npm run styles to update styling)
  • Headings are using Title Case
  • Code blocks have the programming language set to properly highlight syntax and the proper copy directive
  • Have tested with npm run test and resolved all errors
  • Relates to an existing (potentially outdated) blog article? If so please add URL in blog to point to this content.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

This is great! Thank you for writing it up! I left a couple of formatting nits but my main thought was to add a bit more remediation information. I've left suggestions although it might be better to treat them as inspiration rather than exact text! But if you're not keen to spend more time on this then no worries, we can merge as is and add more info later.

content/spin/v3/variables.md Outdated Show resolved Hide resolved
content/spin/v3/variables.md Outdated Show resolved Hide resolved

```console
Handler returned an error: Error::Undefined("no variable for \"<component-id>\".\"YOUR_VARIABLE\"")
```
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add a "how to fix section" here e.g.

To fix this, edit spin.toml and add to the [component.<component-id>.variables] table a line such as <your-variable> = "{{ app-variable }}". See above for more information.

(sorry cannot figure out how to make GH show the above link as a literal Markdown link but hopefully it makes sense what I mean)

content/spin/v3/variables.md Outdated Show resolved Hide resolved
content/spin/v3/variables.md Show resolved Hide resolved
michelleN and others added 5 commits November 14, 2024 19:24
Co-authored-by: itowlson <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Co-authored-by: itowlson <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Co-authored-by: itowlson <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
Signed-off-by: Michelle Dhanani <[email protected]>
@michelleN
Copy link
Member Author

@itowlson I think I addressed all your feedback. Thank you. I also added some headings. Wdyt? I'm 50/50 on em. Thanks again.

@itowlson
Copy link
Contributor

Yeah they are pretty heavy eh. But the extra bit of structure is super useful, it really helps the eye to find the different errors/tips! How about if we made them just bold paragraphs instead of headings?

Another thought was to put them in terms of the errors people were getting rather than the causes/remedies (because someone with a woe doesn't know the cause/remedy yet, all the know is the error). What do you reckon?

Here is how those two changes (together) look on my machine:

image

Better? Worse?

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