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

Some exception that might not work #31639

Merged
merged 9 commits into from
Mar 25, 2024
Merged

Some exception that might not work #31639

merged 9 commits into from
Mar 25, 2024

Conversation

JJ
Copy link
Contributor

@JJ JJ commented Feb 14, 2024

Why:

Well, I've spent the best part of the morning trying to find out why this happened; I got to raise this issue but at any rate it's easier to fix the documentation than fixing the issue. The issue should be right around this line https://github.com/benbalter/jekyll-relative-links/blob/9f05f8524cd974fc151d0d960ae3ad2e4935d3c6/lib/jekyll-relative-links/generator.rb#L52, and I might try to fix it some time in the future, but meanwhile I (or someone else) gets around to do it, gets fixed, and deployed downstream to GitHub pages (which is where I use), this little fix can help lost souls like myself save a bit of time, right? Lots of love to you and all the world in this Valentine day!

Closes:

Does not really close, but mitigates benbalter/jekyll-relative-links#93

What's being changed (if available, include any code snippets, screenshots, or gifs):

Just adding an exception to the example.

Check off the following:

  • I have reviewed my changes in staging, available via the View deployment link in this PR's timeline (this link will be available after opening the PR).

    • For content changes, you will also see an automatically generated comment with links directly to pages you've modified. The comment won't appear if your PR only edits files in the data directory.
  • For content changes, I have completed the self-review checklist.

Well, I've spent the best part of the morning trying to find out why [this](https://github.com/orgs/community/discussions/108272) happened; I [got to raise this issue](benbalter/jekyll-relative-links#93) but at any rate it's easier to fix the documentation than fixing the issue. The issue should be right around this line https://github.com/benbalter/jekyll-relative-links/blob/9f05f8524cd974fc151d0d960ae3ad2e4935d3c6/lib/jekyll-relative-links/generator.rb#L52, and I might try to fix it some time in the future, but meanwhile I (or someone else) gets around to do it, gets fixed, and deployed downstream to GitHub pages (which is where I use), this little fix can help lost souls like myself save a bit of time, right? Lots of love to you and all the world in this Valentine day!
@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Feb 14, 2024
@nguyenalex836 nguyenalex836 added content This issue or pull request belongs to the Docs Content team waiting for review Issue/PR is waiting for a writer's review get started Content related to "Getting Started" doc set and removed triage Do not begin working on this issue until triaged by the team labels Feb 14, 2024
@nguyenalex836
Copy link
Contributor

@JJ Thanks so much for opening a PR! I'll get this triaged for review ✨

Copy link
Contributor

@janbrasna janbrasna left a comment

Choose a reason for hiding this comment

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

@JJ The links you mention in the PR work flawlessly in the GitHub.com interface, it's the Jekyll build that has issues, so the question is whether this Jekyll-specific bit belongs in a general document:

Comment on lines 8 to 20

{% data variables.product.product_name %} will automatically transform your relative link or image path based on whatever branch you're currently on, so that the link or path always works. The path of the link will be relative to the current file. Links starting with `/` will be relative to the repository root. You can use all relative link operands, such as `./` and `../`.

Please note that using links in this way:

```markdown
[this
is a perfectly good relative link](folder/relative.md)
```

might prevent the link to be converted to `folder/relative.html`. Please keep the text link in a single line.

Relative links are easier for users who clone your repository. Absolute links may not work in clones of your repository - we recommend using relative links to refer to other files within your repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

The added comment seems to relate to an issue with actual Jekyll dependency benbalter/jekyll-relative-links#93 for github.io pages builds, yet the reusable is used in many more contexts, mostly related to rendering (not only) markdown files in github.com repository content.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's in the middle of the piece, it's complicated to move it only where it's relevant. One alternative would be to move it out of the piece and into the only place where it's relevant, placing it after the "Relative links" paragraph.

Another option would be doing the same, putting it only where relevant, and moving the "Relative links" sentence to line 3, right after the first sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Come to think of it, right after line 1 would also make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point being the context where the reusable renders in the end:

  1. Repositories / Manage repository settings / Customize your repository / About READMEs:
    https://docs-31639-23d21f.preview.ghdocs.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-readmes

  2. Get started / Writing on GitHub / Start writing on GitHub / Basic formatting syntax:
    https://docs-31639-23d21f.preview.ghdocs.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#relative-links

Since it's Jekyll–specific, esp. a 3rdparty dependency bug–specific, and these are the places to expose it, I can't find a good fit where to mention it — as these seem too generic, and rather GitHub.com–related (if you read the surrounding chapters, mostly mentions READMEs and repository references).

With all the impact this PR has on users not using Jekyll builds, wouldn't fixing the actual cause in the 3rdparty plugin be the easier path?;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, OK, I see what you mean now. I got to that fragment from

, which, as you say, is generic for GitHub, not specific for GitHub pages. There's no specific page for relative links in GitHub pages so what I mentioned above does not make a lot of sense. So here's what we could do: move that patch out to that specific page, clarifying that it will happen only when generating GitHub pages. Would that be acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://pages.github.com/versions/ indicates using v0.6.1 that's one version (and couple years) behind the currently released version so who knows 🤷

(Quickly glancing the changes in the plugin, my bet is that the link handling in GFM/cmark-gfm and jekyll-relative-links differ anyways, so there might be way more discrepancies than this one reported…)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://pages.github.com/versions/ indicates using v0.6.1 that's one version (and couple years) behind the currently released version so who knows 🤷

Exactly my point... I know introducing "temporary" documentation changes for mitigating a bug elsewhere is, at the end of the day, technical debt. I promise I'll set a watch there and come back here and delete what I created when it's fixed.

I had a quick look through the code, but besides involving Ruby, I gathered that it involved a certain amount of refactoring involving what was or was not considered a link, so I quickly left...

Copy link
Contributor

Choose a reason for hiding this comment

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

TL;DR you don't get GFM features from GitHub.com 1:1 in GitHub Pages these days, incl. the newly launched callout syntax et al. even though it uses the same cmark-gfm in the end (but probably not the same version through all the dependencies involved) see: community/16925

With Pages stuck on Jekyll 3.9.x and building newer Jekyll 4+ stacks recommended to be done via custom Actions I can only tell that the disparity of launched features and their absence in GitHub Pages like community/13761 is only going to grow.

Maybe a general note that while the docs cover github.com, the github.io has a different scope and may not support the same features.

Copy link
Contributor

Choose a reason for hiding this comment

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

@JJ FYI there's some limitation in compatibility as the jekyll-relative-links was even reverted from 0.7.0 back to 0.6.1 github/pages-gem#910 — so who knows if there's an update path in the near future anyways…

Copy link
Contributor Author

@JJ JJ left a comment

Choose a reason for hiding this comment

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

Another possible option.

Comment on lines 8 to 20

{% data variables.product.product_name %} will automatically transform your relative link or image path based on whatever branch you're currently on, so that the link or path always works. The path of the link will be relative to the current file. Links starting with `/` will be relative to the repository root. You can use all relative link operands, such as `./` and `../`.

Please note that using links in this way:

```markdown
[this
is a perfectly good relative link](folder/relative.md)
```

might prevent the link to be converted to `folder/relative.html`. Please keep the text link in a single line.

Relative links are easier for users who clone your repository. Absolute links may not work in clones of your repository - we recommend using relative links to refer to other files within your repository.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Come to think of it, right after line 1 would also make sense.

sophietheking
sophietheking previously approved these changes Mar 22, 2024
Copy link
Contributor

@sophietheking sophietheking left a comment

Choose a reason for hiding this comment

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

@JJ 👋 Thanks for opening this PR and contributing to our Docs, we really appreciate it! 💖 I've left a suggestion that updates the wording and moves the explanation to above the example. Would love to hear your thoughts!

data/reusables/repositories/relative-links.md Outdated Show resolved Hide resolved
@nguyenalex836 nguyenalex836 removed the waiting for review Issue/PR is waiting for a writer's review label Mar 22, 2024
It's absolutely perfect. Thanks!

Co-authored-by: Sophie <29382425+sophietheking@users.noreply.github.com>
@sophietheking sophietheking added this pull request to the merge queue Mar 25, 2024
Merged via the queue into github:main with commit aa0884f Mar 25, 2024
38 checks passed
Copy link
Contributor

Thanks very much for contributing! Your pull request has been merged 🎉 You should see your changes appear on the site in approximately 24 hours. If you're looking for your next contribution, check out our help wanted issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content This issue or pull request belongs to the Docs Content team get started Content related to "Getting Started" doc set
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants