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

[Issue #347]: Add next sitemap #354

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[Issue #347]: Add next sitemap #354

wants to merge 6 commits into from

Conversation

rylew1
Copy link
Contributor

@rylew1 rylew1 commented Jun 21, 2024

Ticket

Resolves #347

Changes

  • Add dynamically generated Next.js sitemap

Context for reviewers

image

@rylew1 rylew1 added this to the Next.js App Router milestone Jun 21, 2024
@rylew1 rylew1 requested review from lorenyu and a team June 21, 2024 23:31
@rylew1 rylew1 self-assigned this Jun 21, 2024
Copy link

github-actions bot commented Jun 21, 2024

Coverage report for app

St.
Category Percentage Covered / Total
🟢 Statements 93.1% 81/87
🟢 Branches 82.35% 14/17
🟢 Functions 93.33% 14/15
🟢 Lines 93.59% 73/78

Test suite run success

16 tests passing in 5 suites.

Report generated by 🧪jest coverage report action from 623accb

</head>
<div>healthy</div>
</>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little bit of a separate issue but this fixes an actual a11y issue

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a localization library in this repo - is there a reason not to use it here?

Copy link
Contributor Author

@rylew1 rylew1 Jun 24, 2024

Choose a reason for hiding this comment

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

Sure! Just added it .

I assume because we're not really refining tickets here we're not splitting them up as we might on our normal projects - first inclination is to actually break a task like that out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean to say that you think including content strings using the localization library should be a separate task ticketed separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so because it's getting a bit off track of the intent of this PR

@rylew1 rylew1 requested a review from aligg June 24, 2024 17:24
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.

Add sitemap to Next.js (split ticket)
2 participants