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

Add support for matchers and required language #32

Merged
merged 5 commits into from
Jul 3, 2024

Conversation

JadedBlueEyes
Copy link
Contributor

This implements a similar feature to #29, and it also allows using route matchers - for example [lang=langcode], with the correct file in the params folder. This should also allow [[lang]] to be at more places than just the beginning of the path.

I've added tests for the required lang parameter.

@jasongitmail
Copy link
Owner

Has conflicts b/c the 2nd commit conflicts with v0.14.16 from today, which contains the same changes. I accidentally removed that while troubleshooting another issue today and then re-added it.

@JadedBlueEyes
Copy link
Contributor Author

I've dropped that commit, it should be mergeable now!

@Epoxide
Copy link

Epoxide commented Jul 1, 2024

I've added tests for the required lang parameter.

Happy to see this PR being used instead of mine 👍 Thanks @JadedBlueEyes!

Copy link
Owner

@jasongitmail jasongitmail left a comment

Choose a reason for hiding this comment

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

I need to review this more, but wanted to leave current comments.

What is the goal for how the root path is handled, when the lang param is required?
I.e. Is a dev expected to redirect the root path to one of their preferred required lang paths--e.g. /de? Or does / show duplicate content as one of the /[lang] routes? If so, should one of the href lang's point to the root path instead of its /[lang]? I assume the root path would be considered canonical for that language, correct? I'd be interested to learn what is best practice for SEO in such a situation.

It'd be nice if this had a test at a higher level, to realistically test the actual output if you have ideas on that

src/lib/sampled.ts Outdated Show resolved Hide resolved
src/lib/sampled.ts Show resolved Hide resolved
src/lib/sitemap.ts Outdated Show resolved Hide resolved
src/lib/sitemap.test.ts Show resolved Hide resolved
src/lib/sampled.ts Outdated Show resolved Hide resolved
@Epoxide
Copy link

Epoxide commented Jul 2, 2024

What is the goal for how the root path is handled, when the lang param is required? I.e. Is a dev expected to redirect the root path to one of their preferred required lang paths--e.g. /de? Or does / show duplicate content as one of the /[lang] routes? If so, should one of the href lang's point to the root path instead of its /[lang]? I assume the root path would be considered canonical for that language, correct? I'd be interested to learn what is best practice for SEO in such a situation.

The developer should be expected to redirect to a language. The redirect language should be based off of accept-language in the request headers. If accept-language is missing or it does not exist in the site's supported languages, a default language (most likely English) will be used.

In my PR, root as in / was not present in the sitemap, since this address can never be reached.

@jasongitmail
Copy link
Owner

Thanks @Epoxide, that makes sense.

@JadedBlueEyes
Copy link
Contributor Author

JadedBlueEyes commented Jul 2, 2024

* edit: My mistake, GitHub just moved the annotation

Co-authored-by: Jason <50032291+jasongitmail@users.noreply.github.com>
@jasongitmail
Copy link
Owner

Thanks for the updates @JadedBlueEyes!

@jasongitmail jasongitmail merged commit 21f07f7 into jasongitmail:main Jul 3, 2024
2 checks passed
@jasongitmail
Copy link
Owner

@JadedBlueEyes @Epoxide I'll push a version bump to NPM either later tonight or tomorrow. Going to update the readme tonight and look it over to see if any more testing would be useful.

@jasongitmail
Copy link
Owner

@JadedBlueEyes @Epoxide v0.14.17 is on NPM. Thanks for working on this!

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.

3 participants