-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Redesign: Technical Setup and Project Base #2167
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
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
|
||
| :root { | ||
| --breakpoint-xs: 768px; | ||
| --breakpoint-md: 1440px; |
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.
- --breakpoint-lg is missing
astro/src/styles/tokens/_colors.css
Outdated
| /* Status */ | ||
| --color-success: var(--green-500); | ||
| --color-warning: var(--amber-500); | ||
| --color-error: var(--red-500); |
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.
- These can be removed as replaced by more appropriate semantic tokens (ie --color-text-warning)
astro/src/styles/tokens/_spacing.css
Outdated
| --space-section: var(--space-16); | ||
| --space-component: var(--space-6); | ||
| --space-element: var(--space-4); | ||
| --space-inline: var(--space-2); |
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.
- These can be removed as no longer in use
astro/CONTRIBUTING.md
Outdated
| │ ├── components/ # Reusable UI components | ||
| │ ├── layouts/ # Page layouts | ||
| │ ├── pages/ # Route pages | ||
| │ └── styles/ # Global styles |
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.
- i18n and content folder missing
bjohansebas
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.
LGTM. I don’t have any major concerns other than removing the preinstall script and using devEngines instead. Everything else can be handled in future PRs. Great work!
.vscode/settings.json
Outdated
| }, | ||
| "workbench.colorCustomizations": { | ||
| "titleBar.activeBackground": "#0075d4", | ||
| "titleBar.inactiveBackground": "#0075d4" | ||
| } |
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.
please remove this
| }, | |
| "workbench.colorCustomizations": { | |
| "titleBar.activeBackground": "#0075d4", | |
| "titleBar.inactiveBackground": "#0075d4" | |
| } | |
| } |
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.
Could we use a package instead, rather than keeping our own files in the repository? For example, https://www.npmjs.com/package/@fontsource/jetbrains-mono
. I mention this because I don’t want to have future PRs just to update those files “to update those assets.” Also, it’s a trustworthy package. I’m not saying to do it in this PR, but maybe in the next ones
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.
@bjohansebas I used local fonts here because, based on the design, we only need the light font weight. That means just two font files (191 KB total), so I went with a local setup using optimized woff2 files and proper font-display settings, instead of introducing an additional npm dependency.
I have no strong preference though. Happy to move to @fontsource if that’s the preference. I can quickly handle in this PR, let me know. TY!
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.
Let’s keep it this way in this PR. We can iterate later.
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 haven’t chosen the logo yet, or at least I haven’t been informed, but let’s keep it for now.
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.
Thanks for the info, I will keep a note of this for future checks.
astro/package.json
Outdated
| "type": "module", | ||
| "version": "0.0.1", | ||
| "scripts": { | ||
| "preinstall": "npx only-allow npm", |
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.
Let’s use devEngines, it’s safer and supported by all the major package managers. This should be a requirement.
https://docs.npmjs.com/cli/v11/configuring-npm/package-json#devengines
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'm replacing it! Thanks for the suggestion
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.
@bjohansebas I tried replacing the preinstall script with devEngines, but I understand they serve different purposes. devEngines enforces node/npm versions, but only when npm is used. My original goal with preinstall was to explicitly prevent installs via pnpm/yarn/bun and enforce npm as the package manager. So, for example, pnpm i would fail only because of the preinstall check, not with devEngines.
We can agree this isn’t strictly necessary, since the presence of package-lock.json already implies npm, but I added it as an explicit guardrail.
I'm keeping the devEngines field since it’s useful. Given the above, not sure whether we keep or remove the preinstall script. Let me know! TY
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.
But pnpm also supports it. Yarn can only be used via Corepack, which no longer exists in newer Node.js versions, and I’m not sure whether Bun supports it, although I’d say it probably does.
https://pnpm.io/package_json#devenginesruntime
, but in any case, let’s remove the preinstall script.
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.
done ✅
| title: Developing template engines for Express | ||
| description: Learn how to develop custom template engines for Express.js using app.engine(), with examples on creating and integrating your own template rendering logic. | ||
| menu: advanced | ||
| order: 1 |
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’d like us to stop having this and instead rely on logic to determine the order. If I remember correctly, Starlight handles it based on the menu order. This isn’t a blocker for this PR, just something I’d like to handle in future PRs
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.
Makes sense. I wasn't actually sure about keeping menu and order; we can likely clean them up when building the menu. I'm keeping a note of this.
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.
Not blocking for this PR, but is this really necessary?
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 double-checked this, and I would keep using PostCSS as a minimal layer for CSS feature support. Since Astro already includes PostCSS, this doesn’t add any meaningful overhead. In particular, I’m using @Custom-Media, which isn’t natively supported by browsers yet, and it simplifies working with media queries. We can use intuitive syntax like: @media (--lg-down) { // Style to apply below desktop breakpoint }
On the other hand, I'm removing autoprefixer and cssnano. We can rely on Astro built-in minification, and given our modern-browser support requirements, we don’t need additional prefixing.
wdyt?
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.
Not for this PR, but for future ones, I’d like each language to be handled as a separate JSON. I’m concerned it could end up being a fairly large JavaScript file.
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.
Makes a lot of sense. I'm keeping a note of this for the moment when we'll cover other languages.
| menu: z.string(), | ||
| order: z.number().optional(), |
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.
These two things should ideally not be necessary. For future PRs, we should try to remove them, I understand this is just part of the initial setup, just something to keep in mind.
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.
Agree and noted.
| files: ['**/*.{jsx,tsx}'], | ||
| plugins: { | ||
| 'jsx-a11y': jsxA11y, | ||
| }, | ||
| rules: jsxA11y.configs.recommended.rules, |
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.
For reference, the search will use React, since that’s Orama’s recommendation. It’s the only part that will use React, the rest will be handled with plain JavaScript.
|
Oh, and I almost forgot, please move the license file to the root of the project. It’s not that we forgot about it when we remove the legacy folder |
|
Can we have a preview link? |
Working on it. I’m not sure why Netlify didn’t work for the previews, so I need access to Netlify, which I don’t currently have. I’ll bring this up in today’s meeting Edit: the members who were present don’t have access to Netlify, so I’ll have to wait for @jonchurch to grant me access |
Right! Moving it! Thanks |
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.
RLGTM! The setup and project base is clear, great work @g-francesca!. There quite few things that we can improve, but I think that we can address that on separate PRs and try to land this PR soon to unblock progress on other areas.
Not a blocker but I think that maybe adding a GitHub Action pipeline for the redesign branch might help us to keep smaller PRs in the future and prevent pollution from prettier, etc...
BTW.. I was having some issues to run the build and dev commands:
Ops, right! This was caused by Prettier multiline formatting of TS union types and esbuild having trouble parsing this syntax ins Astro file frontmatter section. To avoid that, I moved types definition to a separate file. @UlisesGascon Can you please try again now? I expect you being able to run dev and build commands.
….com into redesign-astro-setup
|
I just set it so all the branches in this repo will get deployed to netlify for preview. That should trigger deploy previews for all PRs as well I believe |
|
thanks @jonchurch |
|
But could you give me access for anything? It seems you’re the only person there, and I’m not sure if Rand also has access To avoid the bus factor. edit: thanks, now i have access |
Yeah @g-francesca! It is working great now! 🥳
|

Note
There’s no preview link, @bjohansebas working on fixing that
This PR introduces a complete Astro-based rebuild of the Express.js documentation site, establishing the foundational architecture, design system, and UI component library. The work includes:
Project Setup & Infrastructure
Astro 5 Configuration
Development Tooling
.gitignoreand.editorconfigfor better project hygieneDesign System Foundations
Design Tokens
A complete token system following semantic naming conventions, organized in modular CSS files:
Color System
--color-bg-{primary|secondary|tertiary|inverse|mute|success|warning|error}--color-text-{primary|secondary|tertiary|inverse|mute|success|warning|error}--color-border-{primary|secondary|tertiary|inverse|mute|success|warning|error}--color-icon-{primary|secondary|tertiary|inverse}Typography
Spacing
--space-{0.5|1|2|3|4|6|8|12|16}Breakpoints
--xs,--md,--lg,--md-up,--md-down,--lg-downAdditional Tokens
Component Library
Primitive Components
Fully composable, polymorphic UI primitives:
Typography Components
Heading Components:
<H1>through<H5>- Semantic heading componentsBody Text Components:
<Body>- Default paragraph text (16px)<BodyMd>- Medium body text (15px)<BodySm>- Small body text (14px)<BodyXs>- Extra small text (12px)Code Component:
<Code>- Inline code styling with monospace fontFeatures:
<H1 as="h2">renders h2 with H1 stylingprimary,secondary,tertiary,inverse,mute, status colorslight,normal,medium,semibold,boldleft,center,rightLayout Components
Grid System
<Grid>- 12-column responsive grid container<Col>- Grid column with responsive sizingxs,md,lg(1-12)Flexbox Components
<Flex>- Flexbox container with comprehensive propsrow,columnstart,center,end,between,aroundstart,center,end,stretch,baseline<FlexItem>- Individual flex item control1,auto,initial,noneauto,1/4,1/3,1/2,2/3,3/4,fullContainer
<Container>- Centered content wrapperButton Component
Features:
primary,secondaryxs,sm,mdStyling:
Pattern Components
Please note that these are wip.
i18n Infrastructure
Features:
/[lang]/...)Files:
/src/i18n/ui.ts- Language definitions and translation strings/src/i18n/utils.ts-getLangFromUrl()anduseTranslations()helpersRouting & Content Layer
Astro 5 Content Layer API
Features:
src/data/docs/(same will be done for blog posts)Routing Structure
Dynamic routes in src/pages/[lang]/
/[lang]/- Home page with language detection/[lang]/ds-foundations- Living design system documentation - Please note that this is a temporary page, built to provide a living reference for the design system, to facilitate review and test./[lang]/docs/index- Living docs documentation - Please note that this is a temporary page, built to facilitate dynamic routing system review.Notes
Legacy App
Jekyll app has been moved under
legacyfolder with zero changesFigma diffs
There may be some minor differences between the Figma designs and the implemented components. These differences are intentional and result from:
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.