-
Notifications
You must be signed in to change notification settings - Fork 30
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
build(docx): Add Sitemap #2382
build(docx): Add Sitemap #2382
Conversation
Deploying atlantis with
|
Latest commit: |
2499d78
|
Status: | ✅ Deploy successful! |
Preview URL: | https://50a0ac56.atlantis.pages.dev |
Branch Preview URL: | https://sitemap-please-and-thank-you.atlantis.pages.dev |
.circleci/config.yml
Outdated
- run: | ||
name: Create sitemap generator script | ||
command: | | ||
cat \<< 'EOF' > generate-sitemap.js |
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.
Is it possible to roll this script up into just 'generate-sitemap.js' and call it from here?
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.
Also does this generate a sitemap for just storybook or the whole site?
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.
oh sorry this isn't actually ready for review. I was testing some github labels.
also, I was seeing what Cursor would come up with as a first solution which is what all this is.
I'm now considering not doing this in CI at all, and instead using Github Actions to generate the sitemap on merges to master
since there's not really much value in generating a sitemap on every commit/CI run.
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.
Okay! I just saw it was Open while clearing out my email for the day. Curious to see where this lands.
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.
right now I think it's generating a sitemap for storybook only. the first output it had was.... empty, so I told it to check for storybook content and this is as far as it got with that.
Deploying atlantis with
|
Latest commit: |
758b732
|
Status: | ✅ Deploy successful! |
Preview URL: | https://bd435216.atlantis.pages.dev |
Branch Preview URL: | https://sitemap-please-and-thank-you.atlantis.pages.dev |
Deploying atlantis with
|
Latest commit: |
49be2aa
|
Status: | ✅ Deploy successful! |
Preview URL: | https://d7b129d9.atlantis.pages.dev |
Branch Preview URL: | https://sitemap-please-and-thank-you.atlantis.pages.dev |
Deploying atlantis with
|
Latest commit: |
1345672
|
Status: | ✅ Deploy successful! |
Preview URL: | https://d02fdf26.atlantis.pages.dev |
Branch Preview URL: | https://sitemap-please-and-thank-you.atlantis.pages.dev |
Deploying atlantis with
|
Latest commit: |
9960c11
|
Status: | ✅ Deploy successful! |
Preview URL: | https://b8e8c549.atlantis.pages.dev |
Branch Preview URL: | https://sitemap-please-and-thank-you.atlantis.pages.dev |
"/", | ||
"/components", | ||
"/design", | ||
"/hooks", | ||
"/guides", | ||
"/patterns", | ||
"/content", | ||
"/changelog", |
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.
This is not a huge deal, since this is not something that would change frequently, but I was wondering whether this list should come from a shared constant or be generated, so that this test doesn't fail in case some of the routes get added, deleted or updated?
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.
that's a fair point. we don't have any existing asset that we could refer to, so it would have to be something we generate which is pretty similar to the function of the sitemap generation script itself.
I also think this is something that wouldn't change very often, and the cost of updating a test is low enough that I'm in favor of that over creating another script to generate a file.
so if it's alright with you, let's wait until it becomes an annoyance and deal with it then.
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 sitemap generation script looks good! I left a few comments with suggestions to improve readability and testing.
I tried testing it with Cursor, and unfortunately, I'm still not able to access Atlantis documentation (either via @Docs
handle, where I added the link to the preview URL or via @Web
+ preview link):
Query 1 (@Atlantis
) is the prod doc site link.
Query 2(@AtlantisDocs
) is the preview docs site link.

Querying the web with the preview URL.

It does somewhat work if I specifically mention that I'm asking about the Atlantis component.
Querying the web with preview URL + specifying that I'm looking for the Atlantis component:

@@ -30,7 +30,7 @@ | |||
"lint:fix": "npm run lint:js -- --fix && npm run lint:css -- --fix && npm run lint:markdown -- --write", | |||
"lint:js": "eslint --ext .js,.jsx,.ts,.tsx,.mjs,.cjs . --report-unused-disable-directives", | |||
"lint:markdown": "prettier --check '**/*.{md,mdx}' --write", | |||
"merge:dist": "mv ../../storybook-static dist/storybook && mv dist ../../storybook-static", | |||
"merge:dist": "mv storybook-static packages/site/dist/storybook && mv packages/site/dist storybook-static", |
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.
Nice!
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.
This PR looks good! I tested generating sitemap locally with the build script and it works as expected.
Just wanted to note that my comment/issue about prompting with@Docs
and @Web
still stands. However, I don't think it's related to anything in this PR.
Motivations
having a sitemap is a good practice in general. in addition to that, we'd like to help tools like Cursor be able to better parse our documentation.
Changes
Added
added a step in the build script to generate a sitemap for our docs site. this is NOT including all the storybook stories, though perhaps we want to consider adding those too for even more context later.
at the end of the
build
script we will parse our routes and component list to generate the xml ourselves. the output will be placed in thestorybook-static
directory which is roughly equivalent to apublic
folder where assets would go.Changed
Deprecated
Removed
Fixed
Security
Testing
to see the output yourself, run the
npm run build
command from the top level package.json, or thepackages/site
and then thesitemap.xml
file will be instorybook-static
testing the changes with Cursor is a bit un-scientific but what I've been doing is validating the Cloudflare preview url's sitemap.xml, and then using Cursor's
@web
command with the top level docs url then asking it questions about Atlantis componentsif you use prod, I was getting back a message effectively saying it wasn't sure.
if you use the preview url, I'm getting back useful and (mostly) correct details about the Button. it's definitely hallucinating some things like a
medium
size
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.