-
Notifications
You must be signed in to change notification settings - Fork 12
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/add sitemap #1828
Feat/add sitemap #1828
Conversation
🧪 Review environmenthttps://fzxy7czsysut6mvbn3ekdlzzgi0cnyij.lambda-url.ca-central-1.on.aws/ |
- use a common test file for CI runs (`ci.cy.js`) and include both app_pages.cy.js and sitemap.cy.js in it
… alphabetical in each category in fr and en
app/main/views/index.py
Outdated
@@ -257,6 +258,12 @@ def welcome(): | |||
return render_template("views/welcome.html", default_limit=current_app.config["DEFAULT_SERVICE_LIMIT"]) | |||
|
|||
|
|||
# TODO: finish hardcoding the links (or come up with some way to generate these from annotations) | |||
@main.route("/sitemap") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only thing I would change for the future is to make this route /sitemap-plan-du-site or something like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I didn't think of including any of the bilingual routing features in here. We should talk about what end-state we want.
I think there are a few of options:
- multiple routes for each page - this would be the cleanest in my opinion
- multiple routes for each page with a lang parameter
- french and english in one route - this is hard to look at but easier to implement
Multiple routes would look like this:
/sitemap
/plandesite
Multiple routes with a lang parameter would look like this:
/en/sitemap
/fr/plandesite
French and english in one route would look like this:
/sitemap-plandesite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good stuff! works as expected
…ation-admin into feat/add-sitemap
…ot displayed when logged out.
@andrewleith after discussion with Yael, we made a few changes (reasoning in zenhub card):
Thank you! |
Summary | Résumé
This PR adds a sitemap to the website at
/sitemap
. It also adds a macro to handle rendering the sitemap. The macro accepts asitemap
param which is a list of groups which each contain a list of pages and looks like this:Release notes
This WAF rule change PR needs to go out at the same time: