-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[i18nIgnore] New deploy guide navigation component #12735
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
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌑 This pull request will not trigger status changes. Learn moreLunaria automatically ignores changes on specific PRs by adding a ignored keyword in its title. Found: You can change this by either removing the keyword above from the PR's title, or modifying the Tracked FilesNote The notes below indicate what would happen if the pull request is merged when triggering status changes. Since a ignored keyword was found in the PR's title, the status changes indicated below won't be applied.
Warnings reference
|
Co-authored-by: HiDeoo <494699+HiDeoo@users.noreply.github.com>
Co-authored-by: ainurx_78 <34947605+ainurx@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
| gitlab: { file: 'gitlab.svg', padding: '0' }, | ||
| 'google-cloud': { file: 'google-cloud.svg', padding: '.1875em' }, | ||
| firebase: { file: 'firebase.svg', padding: '.1875em' }, | ||
| 'google-firebase': { file: 'firebase.svg', padding: '.1875em' }, |
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.
I can't tell if this is Firebase Studio being Firebase Studio... I don't think we are relying on google-firebase anymore, but I get a type error when I remove this. It's possible this is being used by the Firebase backend guide? It's possible Firebase Studio can't effectively clear a cache and update?
In an ideal world, we don't need this, and I don't know if it's just my setup or if we're still using this somewhere.
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.
It's not a Firebase issue. Well, it is but in our docs. 😄
In BackendGuidesNav.astro, we use:
const logo = isLogoKey(page.id.split('/').pop());And because the backend guide is still named google-firebase.mdx, we're checking if google-firebase is a valid logo key. When we remove google-firebase from src/data/logo.ts this is no longer the case.
So, either we also rename the backend file or we don't rename the logo key yet (and we would need to replace all the logo: firebase changes with logo: google-firebase).
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.
I think we may have hit that exact issue during the call and decided against renaming the icon in this PR in the end? Or am I confusing this one with another one?
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.
Yes, this is the same issue. 😅 If I recall what we said is that updating the other types of guide was maybe out of scope for this PR? But if the issue is just renaming the file and we're fine with that, this is a quick fix I think.
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 think this is a good "quick fix" for now -- we have two entries, one that satisfies each type of guide. When we reconfigure back-end guides to match this setup, we can remove the extraneous google-firebase
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.
@delucis Can you weigh in on whether this "hack" seems OK for the scope of this PR?
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.
UPDATE: I'm updating backend guides to work the same way, so if we merge this as-is, with two entries (firebase and google-firebase) then I should immediately have another PR to merge that will update all the backend guides and will delete the duplicate google-firebase entry.
ArmandPhilippot
left a comment
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.
I don't see any issues to keep both firebase and google-firebase in logos, especially since the accompanying PR is ready. So, everything looks good to me, well done! 🚀
|
Also noting that we will need @yanthomasdev to help with a Lunaria statement here! I believe that the only files that should show as "changed and needing updating" are the guides where
|
|
@sarah11918 I believe this is the code we want! It's only Flightcontrol, SST, Firebase, and Deno that changed, right? |
|
Yes, I think these are the only ones. But I wonder if the followings should be reported as updated:
So, maybe the only ones worth tracking are Flightcontrol and SST because of "via"? But if we prefer to make sure it's okay, yes, that sounds right to me! |
|
I realized that the |
|
Ah yes, good call! I hadn't thought about the filename! Sounds good to me! |
Description (required)
Updates our deployment guides navigation component to work like the other (newer) ones. In particular:
,servicenamelogoto identify the logo icon used;supportsarray of deployment types)~~idand displays service based onsidebar.label(so, a sidebar label prop is now required, and can be fully displayed with localization e.g. "AWS avec SST" since this is now text provided by the translators in the MDX file itself)TODOS:
NICE TO HAVE FOLLOW UPS: