Fix cross-version links in v26.1 documentation#22308
Conversation
✅ Deploy Preview for cockroachdb-api-docs canceled.
|
✅ Deploy Preview for cockroachdb-interactivetutorials-docs canceled.
|
✅ Netlify Preview
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@ebembi-crdb I have discovered how to make image links work using Liquid syntax. Unfortunately the changes currently in this PR don't work, Liquid can't parse them (see build log) The solution appears to be to change e.g. to the following format: I don't know why the "pipe the image syntax into a I have tested this change in my locally running Jekyll instance, and the new syntax works as expected, while allowing us to stop hardcoding the versions. And we can remove the complexity of the |
rmloveland
left a comment
rmloveland
left a comment
There was a problem hiding this comment.
@ebembi-crdb please update the image links as described in my comment and get the build working, and i'll take another look! thank you!
Replace broken Liquid syntax with working path format:
- From: {{ 'images/{{ page.version.version }}/...' | relative_url }}
- To: /docs/images/{{ page.version.version }}/...
Copy changefeed-structure.png from v25.1 and fix all image paths
to use proper Liquid syntax {{ 'images/v26.1/...' | relative_url }}
instead of /docs/images/{{ page.version.version }}/...
|
@ebembi-crdb I see a mix of liquid versions and hard-coded versions in the changes. Can you clarify your approach here? |
| `vehicle_location_histories` | Vehicle location history. | ||
|
|
||
| <img src="/docs/images/{{ page.version.version }}/movr-schema.png" alt="Geo-partitioning schema" style="max-width:100%" /> | ||
| <img src="{{ 'images/v26.1/movr-schema.png' | relative_url }}" alt="Geo-partitioning schema" style="max-width:100%" /> |
There was a problem hiding this comment.
@taroface re: your question, i hypothesize that inside the include file there is not a page.version.version variable for the templating system to reference, and thus the link breaks unless reverted to the old hardcoded method.
Perhaps yet another complexity tax to consider w.r.t. include use going forward
There was a problem hiding this comment.
That would make sense, but we do actually have this variable in include files, if not for images, e.g.:
There was a problem hiding this comment.
oh interesting .... maybe I was wrong! need to revisit and test this again more carefully. for now I LGTM'd this PR since it's building at least, so your KLs PR for the release can be unblocked
| - A query is executed against node 2 to read from table 3. | ||
|
|
||
| <img src="/docs/images/{{ page.version.version }}/perf_tuning_concepts1.png" alt="Perf tuning concepts" style="max-width:100%" /> | ||
| <img src="{{ 'images/v26.1/perf_tuning_concepts1.png' | relative_url }}" alt="Perf tuning concepts" style="max-width:100%" /> |
There was a problem hiding this comment.
@ebembi-crdb i wrote in 4dd3cd2#r2738108844 about why the new syntax might break in an include
however in my testing this type of link should work in a non-include file, so I'd expect it to work here. But perhaps this one also broke because it's in the architecture subfolder? (If yes this strengthens my argument against subfolders - see also https://cockroachlabs.atlassian.net/browse/DOC-3720)
curious what you saw, i'm also trying this on my local machine now
There was a problem hiding this comment.
@ebembi-crdb this is working locally for me, i think this and all other changes to the link syntax that are not in include files can be reverted to the /docs/images/{{ page.version.version }}/*.png syntax. Please try pushing a change with only the _include files reverted to the old syntax, and keep the new syntax in all other files
rmloveland
left a comment
rmloveland
left a comment
There was a problem hiding this comment.
LGTMing to unblock #22325
as discussed in my comments below, i think we can use the new link syntax everywhere except include files, however i don't think this PR should block anyone's v26.1 work so please merge this ASAP given that the build is green. we can revisit after the release
3 participants
Summary
Test Plan