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

docs(router): NavLink wording improvements #10380

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Mar 31, 2024

Just trying to improve the wording in our docs to make them easier to read and understand

@Tobbe Tobbe added the release:chore This PR is a chore (means nothing for users) label Mar 31, 2024
@Tobbe Tobbe added this to the chore milestone Mar 31, 2024
@Tobbe Tobbe added release:docs This PR only updates docs and removed release:chore This PR is a chore (means nothing for users) labels Mar 31, 2024
@Tobbe Tobbe added the changesets-ok Override the changesets check label Apr 2, 2024
@Tobbe Tobbe requested a review from jtoar April 2, 2024 07:55
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

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

Nice! Reads like an improvement to me. Left one comment and two minor edits. All small things so feel free to address if you want but optional

<!-- When match "/" -->
<!-- Will match "/" -->
Copy link
Contributor

Choose a reason for hiding this comment

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

The original wasn't clear, but I think the spirit of it is lost if we replace it with just "will". I think "when" was supposed to be short for something like adds "activeLink" when the URL matches "/". Maybe we could do will add "activeLink" when the URL matches "/"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to keep as close as possible to the existing docs, just make them easier to read and understand.

With that in mind I like your first suggestion best. It's probably more inline with what the original author wanted to express. So I'll go with that for this PR.

That said, it's actually wrong now. But I'll get this merged, and then follow up with a different PR that explains what I mean.

Copy link
Member Author

@Tobbe Tobbe Apr 2, 2024

Choose a reason for hiding this comment

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

Follow up here: #10401

docs/docs/router.md Outdated Show resolved Hide resolved
docs/docs/router.md Outdated Show resolved Hide resolved
@Tobbe Tobbe merged commit 2bef30c into redwoodjs:main Apr 2, 2024
45 checks passed
@Tobbe Tobbe deleted the tobbe-nav-link-wording branch April 2, 2024 09:49
dac09 added a commit to dac09/redwood that referenced this pull request Apr 2, 2024
…nto feat/register-middleware

* 'feat/register-middleware' of github.com:dac09/redwood:
  docs(router): NavLink wording improvements (redwoodjs#10380)
Josh-Walker-GM pushed a commit that referenced this pull request Apr 2, 2024
Co-authored-by: Dominic Saadi <dominiceliassaadi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:docs This PR only updates docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants