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

Update multi-page navigation to allow custom path prefix #136

Closed
wants to merge 1 commit into from

Conversation

alex-ju
Copy link

@alex-ju alex-ju commented Oct 31, 2019

Currently if the tech-docs is deployed on a path (e.g. https://alphagov.github.io/api-catalogue/) the navigation links are broken as they are generated from root (/). One way to avoid this without affecting current single-page or multi-page configuration is to define a path_prefix in config that is prepended to each link.

This is an attempt to fix #128 which is currently blocking any deployment for the api-catalogue.

I'm open to any other suggestions that will unblock this - especially since I'm not familiar with this project or Middleman.

Note: I needed to refactor the conditional statements as linter was complaining about the code having too many lines (Metrics/BlockLength: Block has too many lines. [31/25])

Currently if the tech-docs is deployed on a path (e.g. `https://alphagov.github.io/api-catalogue/`) the navigation links are broken as they are generated from root (`/`). One way to avoid this without affecting current single-page or multi-page configuration is to define a `path_prefix` in config that is prepended to each link.
@alex-ju alex-ju changed the title Update multi-page navigation to respect root host Update multi-page navigation to allow custom path prexit Oct 31, 2019
@alex-ju alex-ju changed the title Update multi-page navigation to allow custom path prexit Update multi-page navigation to allow custom path prefix Oct 31, 2019
Copy link
Contributor

@freesteph freesteph left a comment

Choose a reason for hiding this comment

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

Could we add some tests for this?

@alex-ju
Copy link
Author

alex-ju commented Oct 31, 2019

Could we add some tests for this?

I thought a lot about that, but not sure where it fits as the test suite currently relies on one configuration file. This change will require a separate config and not sure where to accommodate that. Open to suggestions.

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

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

I think what you've built here has already been done in another PR: #79

I don't see how this differs to http_prefix 🤔 and if http_prefix isn't doing what we want, it's probably a bug in its implementation and we should fix that rather than add a new property.

As for tests, we can define separate config & tests inside helpers_spec.rb.

I may be missing some context here - let me know if you'd like to pair on it this week! 😊

@alex-ju
Copy link
Author

alex-ju commented Dec 9, 2019

I don't see how this differs to http_prefix 🤔 and if http_prefix isn't doing what we want, it's probably a bug in its implementation and we should fix that rather than add a new property.

Ha, I haven't thought about that - I saw it as more of a way to specify the protocol (http/https). I'll give it a go, if it doesn't work I would really appreciate a hand, thank you!

@ChrisBAshton
Copy link
Contributor

I agree the naming could be better!

@alex-ju
Copy link
Author

alex-ju commented Dec 10, 2019

As mentioned in the comments – this seems to have been addressed already.

@alex-ju alex-ju closed this Dec 10, 2019
@alex-ju alex-ju deleted the fix-multi-page-navigation-for-custom-host branch December 10, 2019 11:23
@alex-ju
Copy link
Author

alex-ju commented Jan 13, 2020

This is still a problem – maybe #84 will fix it.

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.

Navigation is broken when deploying on a path
3 participants