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(v2): allow relative URLs in URISchema #3449

Merged
merged 1 commit into from
Sep 29, 2020

Conversation

tommilligan
Copy link
Contributor

@tommilligan tommilligan commented Sep 14, 2020

Motivation

Currently, relative links are invalid when configuring navbar items.

We currently run our documentation at a sub-path of our top level domain, https://example.org/docs. For the logo, it is valid to add a relative link up to our homepage:

module.exports = {
  themeConfig: {
    navbar: {
      logo: {
        href: "..",
        target: "_top",
        src: 'img/logo_black.svg',
        srcDark: 'img/logo_white.svg',
      }
...

This behaves well, linking up to the relative parent page and forcing a full page reload.

I would also like the following configuration to be valid, for navbar items:

module.exports = {
  themeConfig: {
    navbar: {
      items: [{
            href: "../sibling_path",
            target: "_top",
            label: "Sibling Application",
            position: "right"
      }]
...

This is currently not possible to achieve within themeConfig.navbar.items, as an items href value is more strictly validated with URISchema, disallowing relative URLs.

A validation error occured.
The validation system was added recently to Docusaurus as an attempt to avoid user configuration errors.
We may have made some mistakes.
If you think your configuration is valid and should keep working, please open a bug report.

ValidationError: "navbar.items[2].href" does not match any of the allowed types

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Validation unit tests were updated to demonstrate that relative URLs can be configured for URIScheme, which is used to validate ThemeConfig.

The existing test case was updated to a legitimately invalid URL.

This change will be less strict than master's validation, as documented by Joi.

Related PRs

I do not believe this requires any documentation changes.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 14, 2020
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 ready!

Built without sensitive environment variables with commit bd93556

https://deploy-preview-3449--docusaurus-2.netlify.app

@tommilligan
Copy link
Contributor Author

Tested locally by manually building docusaurus-utils-validation and copying the resulting lib into my existing project's node_modules tree. My navbar items with relative href links were generated correctly, and behave as expected.

@tommilligan
Copy link
Contributor Author

My original issue has been worked around as per discussion here: #3453 (comment)

I think this PR is still valid for adding relative links and making the href property work as expected.

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Hi.

Honestly, I'm not confident merging this PR right now so I'm going to close it.
I don't think relative URLs should be supposed in these 2 options anyway, and actually think we should be more strict, as a relative URL will depend on the location you are currently on, while the navbar items URLs should probably be "constant" for all pages of the site

Note that the "to" attribute is currently validated much less strictly, as it accept any string, and looks like it could do the job for your usecase, so it can be a good workaround until I know better how to solve these problems and don't rush on a solution.

Still interested to understand better your usecase so if you have a specific need that is not covered properly, a repro and links to a live site to understand better would be welcome.

@slorber slorber closed this Sep 29, 2020
@slorber
Copy link
Collaborator

slorber commented Sep 29, 2020

Ok, don't think it'll be bad to be a bit less strict (as we accept any "to" anyway) so I'll merge it for now, hope to not regret this later.

@slorber slorber reopened this Sep 29, 2020
@slorber slorber changed the title fix(utils-validation): allow relative URLs in URISchema fix(v2): allow relative URLs in URISchema Sep 29, 2020
@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Sep 29, 2020
@slorber slorber merged commit fe78cbe into facebook:master Sep 29, 2020
@lex111 lex111 added this to the v2.0.0-alpha.65 milestone Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants