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

feat: Localize file-based dynamic routes #654

Merged

Conversation

tommasongr
Copy link
Contributor

@tommasongr tommasongr commented Nov 23, 2022

Summary

Adds localized routes under the _routes folder. This is done by creating all the possible slugs on the fly based on site.config.available_locales.

Every route will set I18n.locale and r.params[:locale] to the corresponding value extracted from the url slug.

A possible missing piece might be a dedicated config for opting-in/out from the behavior. However, I think that forcing the locale on the file frontmatter could be a good enough alternative and possibly a more granular one.

This feature adds the ability to have localized routes under the _routes folder.

This is done by creating all the possible slugs on the fly based on the site.config.available_locales.

Every routes will have I18n.locale already configured and the locale passed also through r.params[:locale]
@jaredcwhite jaredcwhite added the high priority This should get addressed as soon as possible. label Nov 23, 2022
@jaredcwhite jaredcwhite added this to the 1.2 milestone Nov 23, 2022
Copy link
Member

@jaredcwhite jaredcwhite left a comment

Choose a reason for hiding this comment

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

@tommasongr Very nicely done. I had one little code nitpick, but otherwise LGTM. 👍

bridgetown-routes/lib/bridgetown-routes/manifest.rb Outdated Show resolved Hide resolved
@jaredcwhite
Copy link
Member

@tommasongr Looks like some Rubocop checks are failing. I'd be fine with adding some # rubocop:todo comments in if need be, but want to get your eyes on it first.

@tommasongr
Copy link
Contributor Author

@jaredcwhite Thanks for reporting it. Yeah, I was aware of that. I fixed all the "real" offenses, but I wasn't sure how to treat the Metrics one. Anyway I'll comment/fix all the remaining offenses and push a final commit. Give me a sec.

@tommasongr
Copy link
Contributor Author

@jaredcwhite I resolved three offenses in manifest.rb and manifest_router.rb by extracting some code to external methods and I marked as rubocop:todo an offense in test_routes.rb. I hope the new structure is fine with you. Let me know.

Copy link
Member

@jaredcwhite jaredcwhite left a comment

Choose a reason for hiding this comment

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

LGTM!

@jaredcwhite jaredcwhite merged commit 1452beb into bridgetownrb:main Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high priority This should get addressed as soon as possible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants