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 links for single language sites #1744

Merged
merged 1 commit into from Feb 1, 2024
Merged

Fix links for single language sites #1744

merged 1 commit into from Feb 1, 2024

Conversation

LisaFC
Copy link
Collaborator

@LisaFC LisaFC commented Nov 16, 2023

@LisaFC
Copy link
Collaborator Author

LisaFC commented Nov 16, 2023

See discussion with @lyind in #1486

Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

This breaks the page-meta links for some pages on the OTel website, for example the mission page. I'll investigate further as soon as I can.

@chalin
Copy link
Collaborator

chalin commented Nov 16, 2023

@LisaFC et al. - do you have a reference to a public site that exhibits the broken page-meta links? Or do you have a simple test case to reproduce the problem?

@chalin
Copy link
Collaborator

chalin commented Nov 16, 2023

Ah, nm, found one :)

@@ -1,5 +1,5 @@
{{ if .File }}
{{ $pathFormatted := replace .File.Path "\\" "/" -}}
{{ $pathFormatted := strings.TrimPrefix hugo.WorkingDir $.File.Filename -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The line your are changing was added in support of builds under Windows:

Removing the line will introduce a regression IMHO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, our tests are passing under windows, so I'll assume that this is still ok. If not, we can address the regression later.

@LisaFC
Copy link
Collaborator Author

LisaFC commented Nov 16, 2023

Ok, let me take a further look

@LisaFC
Copy link
Collaborator Author

LisaFC commented Nov 16, 2023

If you remove the overridden partial in this project you should see the problem: https://github.com/LisaFC/monolang

@chalin
Copy link
Collaborator

chalin commented Nov 17, 2023

I got the monolang repo working with Docsy at HEAD (using simple front matter) but, regardless, I'm feeling that your fix is the right way to go! 🎉 I have a tweaked version locally, and will share it + more feedback when I'm back.

@chalin chalin mentioned this pull request Nov 29, 2023
33 tasks
@chalin chalin self-assigned this Dec 8, 2023
@chalin chalin added this to the 23Q4 milestone Dec 8, 2023
@chalin chalin modified the milestones: 23Q4, 24Q1 Jan 11, 2024
Fixes #981
Fixes #138

Requires Hugo 0.112.0
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Great work! This definitely seems like the right way to fix this. There is some cleanup in the code that I'd like to do, but I'll address that in a followup PR, and add a statement about this being a breaking change to the CHANGELOG.

@@ -1,5 +1,5 @@
{{ if .File }}
{{ $pathFormatted := replace .File.Path "\\" "/" -}}
{{ $pathFormatted := strings.TrimPrefix hugo.WorkingDir $.File.Filename -}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, our tests are passing under windows, so I'll assume that this is still ok. If not, we can address the regression later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants