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! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughReplaces the previous text-based logo glyph with new SVG paths and a changed viewBox in AppLogo; adds a new AppMark component (small SVG) and uses it for the mobile home link while enlarging the desktop AppLogo and removing the adjacent "npmx" text label. Adjusts spacing and badge positioning in AppHeader and the index page to suit the larger desktop logo. Updates pwa-assets config to reference logo-icon.svg and test fixtures to match; adds accessibility tests for AppMark. 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
@alexdln Changes done!
Here are screenshots, though you might want to open them in separate tabs and flip back/forth between them. Of course, let me know if you have any additional feedback. Before/after screenshotsHomepageBefore
After
HeaderBefore
After
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/AppMark.vue (1)
9-17: Align SVG accessibility semantics.
aria-hidden="true"means assistive tech will ignore this SVG, so the<title>on Line 16 is effectively unused. Either remove the<title>or expose the SVG semantics intentionally (e.g. noaria-hidden, with explicit labelling).
ℹ️ 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 (4)
- test/unit/server/utils/file-tree.spec.ts
- test/nuxt/a11y.spec.ts
- app/components/AppLogo.vue
- pwa-assets.config.ts
Looks like both |
I see the following templates:
None of them seem to refer to any On the other hand, I do see that So I don't immediately see any additional OG image work that needs to be done as part of this PR specifically, but @harlan-zw please let me know if I missed something! 🙏🏻 |
|
Sorry, I was going based on the previous PR being merged, didn't realize it got reverted #1654. I can swap around the paths when I recreate it so please ignore me 😄 |
No problem! 🙌🏻 |
4 participants
🔗 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.