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

fix: broken link on icons readme page in patternhub #2935

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bruno-sch
Copy link
Collaborator

@bruno-sch bruno-sch commented Jul 30, 2024

Proposed changes

Link is corrected. Fixes #2930

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (fix on existing components or architectural decisions)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Further comments

@bruno-sch bruno-sch enabled auto-merge July 30, 2024 09:01
@github-actions github-actions bot added 📕documentation Improvements or additions to documentation 🏗foundations Changes inside foundations folder labels Jul 30, 2024
Copy link
Contributor

@mfranzke
Copy link
Member

mfranzke commented Jul 30, 2024

@nmerget even though that @bruno-sch has a valid solution here, it might not be the best, as I would assume that we should link to an .md file instead of the resulting path, shouldn't we ?

@nmerget
Copy link
Member

nmerget commented Jul 30, 2024

If we want to enable the link in GitHub and Patternhub, we can't use relative paths. They have to be absolute :(

@mfranzke
Copy link
Member

mfranzke commented Jul 30, 2024

If we want to enable the link in GitHub and Patternhub, we can't use relative paths. They have to be absolute :(

por que? There seems to be some logic that let's those items appear as custom-icons path in the main navigation link. Why can't we use the same magic powder to even also rewrite any link to an .md file even for inline content?

@@ -23,4 +23,4 @@ You can add an icon before or after a tag, by adding an `data-` attribute to you

If you have custom icons and want to use them for foundations and/or in components, you need to generate a **woff2** file.

[More information](./CustomIcons.md)
[More information](./custom-icons)
Copy link
Member

@mfranzke mfranzke Jul 30, 2024

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mfranzke and @nmerget

That's basically true. In the end, this also applies to all other internal links here: If links point to paths with the file extension .md, they work in Github, but not in Patternhub. And vice versa.

A possible solution would be to have links in the md files end with the appendix .md. However, when copying these files to Patternhub via node-script, remove .md from links.

What do you think?

Copy link
Member

@mfranzke mfranzke Aug 22, 2024

Choose a reason for hiding this comment

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

@mfranzke and @nmerget

That's basically true. In the end, this also applies to all other internal links here: If links point to paths with the file extension .md, they work in Github, but not in Patternhub. And vice versa.

A possible solution would be to have links in the md files end with the appendix .md. However, when copying these files to Patternhub via node-script, remove .md from links.

What do you think?

I agree with what you're suggesting. In either case we would need some kind of transformation of these links to another format. And as the codebase out of GitHub is displayed at github.com out of the box, but does get some kind of transformation for being displayed in "patternhub", we should rewrite the URL in the latter step / setup.

Copy link
Member

Choose a reason for hiding this comment

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

as aligned with @bruno-sch: the links within markdown need to be correct and during the transformation for GitHub Pages those links would need to get transformed as well to the resulting structure.

@nmerget nmerget assigned mfranzke and unassigned nmerget Aug 5, 2024
@mfranzke mfranzke added this to the Guidelines 3.0 RC milestone Aug 5, 2024
@mfranzke mfranzke removed their assignment Aug 5, 2024
@mfranzke mfranzke marked this pull request as draft September 22, 2024 11:09
auto-merge was automatically disabled September 22, 2024 11:09

Pull request was converted to draft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📕documentation Improvements or additions to documentation 🏗foundations Changes inside foundations folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

more information link doesn't work
3 participants