-
-
Notifications
You must be signed in to change notification settings - Fork 74
docs(readme): use h3 headers for options #471
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
Conversation
Also changed wording of routeParams documentation to indicate dynamic behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm I suggested we use h4 (or another header) so options can be linked.
I suggest to wait for @Fdawgs feedback before continuing on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'd go with h3 though to align with the main Fastify repo's style.
Co-authored-by: Jean <[email protected]> Signed-off-by: Ben McLean <[email protected]>
@jean-michelet and @Fdawgs, I have made the requested changes. Descriptions are now on new-lines and I have used h3 instead of h4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you plz merge if you agree @Fdawgs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Ben-at-Catalyst, one last thing, could you sort the indentation please? The indentation of the text and js blocks following these new headers is no longer needed.
Was obviously needed when they were bullet points.
Done. @Fdawgs I didn't realize they were just there to make the list blocks work properly :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I wasn't able to find the
routeParams
option on the README, I had been searching for "dynamic", "route segment", etc. I opened an issue and was informed of therouteParams
option.This PR should help future users search for that option as I did.
@jean-michelet Also suggested that I change the options to use h4 headings instead of plain list entries, so that they can be linked, so I've done that here as well.
Closes #465
Checklist
npm run test && npm run benchmark --if-present
and the Code of conduct