Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a new AppMark component and updates header and index layouts to use AppMark on mobile while showing a larger AppLogo on desktop; removes the textual “npmx” label from header. Replaces AppLogo SVG geometry (paths and viewBox) and removes background rect/text. Swaps pwa-assets config entries to reference logo-icon.svg. Tests updated to include AppMark accessibility checks and to reflect the renamed asset in the file-tree fixture. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
would you mind resolving the conflict? 🙏 |
|
@danielroe Fixed! 7a0fc21 updated Then, as before, I renamed |
|
The test failures seem unrelated? |
|
seems a takumi/og image flakiness (cc: @harlan-zw) |
|
I'm sorry! reverted the og image change due to the flakiness (was also apparent per-route and on other PRs ...) |
Assuming it is an issue with takumi as @danielroe said, I investigated the aforementioned message: Here's what the dependency chain looks like:
We use takumi to generate images, and takumi uses mimalloc_rust as its memory allocator. But there's a recent issue on mimalloc_rust purpleprotocol/mimalloc_rust#153 where people are reporting:
And in fact, the transition from mimalloc_rust 0.1.47 to 0.1.48 seems significant, because mimalloc_rust commit 747b5b bumps the version of the underlying C library (Microsoft's mimalloc) from v2 to v3. Could this be the cause of our problem? Maybe, but it doesn't look like a recent dependency change would have caused it. The version of mimalloc we depend on is based on the version that takumi depends on, and takumi bumped to mimalloc 0.1.48 in commit be28f9f, made on 31 Aug 2025. (cc @andrew, this was a great use of git-pkgs, thanks) So it might be that the above mimalloc change is causing our issue, but if so, it's either happening intermittently, or only with certain inputs. @danielroe Should I just create a new issue for this, so we can keep track of it? |
|
@danielroe No problem, rebased! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/AppLogo.vue (1)
7-43: LGTM!The updated SVG implementation looks good:
- Uses
viewBoxwith explicit dimensions for consistent scalingfill="currentColor"enables flexible colour theming- The accent slash uses
var(--accent)CSS variable appropriatelyaria-hidden="true"with<title>is a valid pattern for decorative logosMinor note (optional): The
stroke-width=".26458"attributes on all paths have no effect since nostrokecolour is defined. These appear to be leftover from SVG export tooling and could be removed for cleaner output, but they cause no functional issues.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
public-dev/logo-icon.svgis excluded by!**/*.svgpublic-dev/logo-mark.svgis excluded by!**/*.svgpublic-dev/logo.svgis excluded by!**/*.svgpublic-staging/logo-icon.svgis excluded by!**/*.svgpublic-staging/logo-mark.svgis excluded by!**/*.svgpublic-staging/logo.svgis excluded by!**/*.svgpublic/logo-icon.svgis excluded by!**/*.svgpublic/logo-mark.svgis excluded by!**/*.svgpublic/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
app/components/AppHeader.vueapp/components/AppLogo.vueapp/components/AppMark.vueapp/pages/index.vuepwa-assets.config.tstest/nuxt/a11y.spec.tstest/unit/server/utils/file-tree.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- app/pages/index.vue
- test/unit/server/utils/file-tree.spec.ts
- app/components/AppMark.vue
- test/nuxt/a11y.spec.ts
- app/components/AppHeader.vue
This originates from the original |
|
@vladh Wow, excellent work, thank you for all the changes and unifications A bit about details:
I'm aware that the slash is too low at the top. All the fonts create a different experience and habit, and I felt the difference here - you get *that* feeling that something is wrong, but you can't explain what The stage text looks too far away when it's in the header. It's like it's at a different distance on the home page where it looks comfortable, and in the header - where it's a bit too far away |
|
Hey @alexdln, thanks for the feedback!
Am I correct in understanding that you would like the slash to be a little bit taller? Initially, I thought it was too tall and a bit distracting — of course, the font designer will have chosen an appropriate height for normal text, but it felt a bit tall in a “display” scenario like this logo. But, I may have overdone the change, making it too short. Maybe I can find a middle ground — does that sound good?
No problem, I'll make the spacing of the environment text a bit tighter in the header. |
Yes, I think a good option would probably be somewhere in between. Maybe make the distance between the top and bottom of the lowercase roughly equal. Maybe just a liiittle bit more at the top |
On it! 🫡 |
There was a problem hiding this comment.
Can you please make the same distance at the top and bottom, otherwise it will always be sth like mt-0.125

🔗 Linked issue
It was easier to just create the PR to demonstrate the fix — hope that's okay.
🧭 Context
The logo, on the homepage, and in the navigation, had various issues. Most significantly, the kerning and spacing seemed wrong to me.
📚 Description
I've create a new logo SVG and applied it throughout the website. Here are the benefits, which are visible in the below screenshots. Please look at the screenshots in full size to be able to see the differences properly.
<path>s, ensuring the logo is rendered identically across all browsers.AppLogoand anAppMarkcomponent, making it easier to use either the full logo, or just the “./” mark.AppLogonow contains the full logo, allowing it to be more easily used across the website.AppLogopreviously incorrectly had a background colour and rounded corners, which has been fixed.logo.svgfile has been renamed tologo-icon.svg, creating space for the newlogo.svgwhich contains the full logo, andlogo-mark.svgwhich contains just the “./” mark on a transparent background. This should be handy for future use, in eg marketing materials.