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: add relative links to various file references #1392

Merged
merged 2 commits into from
Jan 21, 2025

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Jan 21, 2025

Context

Was just reading through the docs here on GH and saw a lot of references to files that could have relative links to those files for easy click through

Details

  • when a specific file in the codebase was referenced, link to it

    • did not otherwise change the formatting of the content and left it as-is (e.g. bold text)
  • also fix a few incorrect (moved?) locations:

    • HomeDBManager.ts now in homedb/ dir
    • a few files missing extensions
    • some incorrect extensions (e.g. was .ts but it's actually still .js)
    • having links to the files makes it much easier to find these issues
  • and standardize one file reference that was different from the rest ("Python sandbox in this code:" -> "Python sandbox in [...]")

Related issues

Made some other docs improvements in #1391

Has this been tested?

  • [n/a] 👍 yes, I added tests to the test suite
  • [n/a] 💭 no, because this PR is a draft and still needs work
  • 🙅 no, because this is not relevant here
  • [n/a] 🙋 no, because I need help

I double-checked all the links to ensure they worked properly. Also my code editor (VS Code) would autocomplete the relative paths correctly when I typed them as another form of check.

- when a specific file in the codebase was referenced, link to it
  - did not otherwise change the formatting of the content and left it as-is (e.g. bold text)

- also fix a few incorrect (moved?) locations:
  - `HomeDBManager.ts` now in `homedb/` dir
  - a few files missing extensions
  - some incorrect extensions (e.g. was `.ts` but it's actually still `.js`)

- and standardize one file reference that was different from the rest ("Python sandbox in this code:" -> "Python sandbox in [...]")
Copy link
Collaborator

@fflorent fflorent left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thank you!

Some little improvements you may add:

  • You may use absolute link (instead of using ../path/to/resource.ext, using /path/to/resource.ext), but if the document is moved elsewhere, the links are still valid;
  • In documentation/overview.md ⇒ Changes to documents, I see that sandbox/grist/useractions.py has no link

Maintains data for a metadata table, making it available as observable arrays of `MetaRowModel`s. The difference between metadata tables and user tables is that the Grist app knows what’s in metadata, and relies on it for its functionality. We also assume that metadata tables are small enough that we can instantiate [observables](https://github.com/gristlabs/grainjs/blob/master/docs/basics.md#observables) for all fields of all rows.
* `app/client/models/`**`DataTableModel.js`**
* [`app/client/models/`**`DataTableModel.js`**](../app/client/models/DataTableModel.js)
Maintains data for a user table, making it available as `LazyArrayModel`s (defined in the same file), which are used as the basis for `koDomScrolly` (see `app/client/lib/koDomScrolly.js` below).
Copy link
Collaborator

Choose a reason for hiding this comment

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

You might add a link here too:

Suggested change
Maintains data for a user table, making it available as `LazyArrayModel`s (defined in the same file), which are used as the basis for `koDomScrolly` (see `app/client/lib/koDomScrolly.js` below).
Maintains data for a user table, making it available as `LazyArrayModel`s (defined in the same file), which are used as the basis for `koDomScrolly` (see [`app/client/lib/koDomScrolly.js`](/app/client/lib/koDomScrolly.js) below).

Copy link
Contributor Author

@agilgur5 agilgur5 Jan 21, 2025

Choose a reason for hiding this comment

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

I actually specifically didn't add it to this one since this refers to "below", which has both a link and a description. As in, linking this one may make a reader miss the description, which goes against the intent of the text -- the intent of the text here is to make you scroll down rather than read the code

@agilgur5
Copy link
Contributor Author

Nice catch! I added that in c61c8a7

  • You may use absolute link (instead of using ../path/to/resource.ext, using /path/to/resource.ext), but if the document is moved elsewhere, the links are still valid;

Hmm, IIRC this might only be valid for GH-flavored markdown rendered on GH. My editor doesn't recognize this and tries opening an actual absolute link on my FS. So there's a trade-off to using absolute links -- let me know if you still want me to make that change

@fflorent
Copy link
Collaborator

Hmm, IIRC this might only be valid for GH-flavored markdown rendered on GH. My editor doesn't recognize this and tries opening an actual absolute link on my FS. So there's a trade-off to using absolute links -- let me know if you still want me to make that change

From my prospective, it is acceptable as it is.

@fflorent
Copy link
Collaborator

I approved your PR, but please note that I don't have the rights for merging. You should wait for someone at GristLabs to do that.

Copy link
Member

@paulfitz paulfitz left a comment

Choose a reason for hiding this comment

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

Thanks @agilgur5 (and @fflorent for reviewing!)

@paulfitz paulfitz merged commit b12b5d6 into gristlabs:main Jan 21, 2025
12 checks passed
@agilgur5 agilgur5 deleted the docs-relative-code-links branch January 21, 2025 19:07
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