Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
…xdln/npmx.dev into feat/new-sections-about-page
app/pages/about.vue
Outdated
| import LogoVercel from '~/assets/logos/sponsors/vercel.svg' | ||
| import LogoVoidZero from '~/assets/logos/sponsors/void-zero.svg' | ||
| import LogoNuxt from '~/assets/logos/oss-partners/nuxt.svg' | ||
| import LogoOpenSourcePledge from '~/assets/logos/oss-partners/open-source-pledge.svg' | ||
| import LogoOxC from '~/assets/logos/oss-partners/oxc.svg' | ||
| import LogoRolldown from '~/assets/logos/oss-partners/rolldown.svg' | ||
| import LogoStorybook from '~/assets/logos/oss-partners/storybook.svg' | ||
| import LogoVite from '~/assets/logos/oss-partners/vite.svg' | ||
| import LogoVitest from '~/assets/logos/oss-partners/vitest.svg' | ||
| import LogoVue from '~/assets/logos/oss-partners/vue.svg' |
There was a problem hiding this comment.
we could probably just drop these in the public/ directory and reference them without importing them (importing them makes them part of the build, which is probably unnecessary)
There was a problem hiding this comment.
In public they will be cached and here the question is whether we expect logos to change (like void zero changed for all its utilities) 🤔
📝 WalkthroughWalkthroughAdds Sponsors and OSS Partners sections to the about page: imports SPONSORS and OSS_PARTNERS logo data, adds LogoImg and LogoList components, and renders those lists with separators and theme-aware logo variants. Replaces several LinkBase usages with plain anchor elements and introduces a deprecated Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
Sorry, I forgot about the light theme 🫠 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
app/assets/logos/oss-partners/nuxt.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/open-source-pledge.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/oxc.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/rolldown.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/storybook.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/vite.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/vitest.svgis excluded by!**/*.svgapp/assets/logos/oss-partners/vue.svgis excluded by!**/*.svgapp/assets/logos/sponsors/vercel.svgis excluded by!**/*.svgapp/assets/logos/sponsors/void-zero.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
app/components/CallToAction.vueapp/components/Link/Base.vueapp/pages/about.vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.json
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/pages/about.vue (1)
141-141:⚠️ Potential issue | 🟡 MinorRemove the per-button focus-visible utility class.
Line 141 adds
focus-visible:outline-accent/70on a<button>, which conflicts with the global focus-visible pattern and creates inconsistent focus styling.Suggested fix
- class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="cursor-pointer inline-flex items-center gap-2 p-1.5 -mx-1.5 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"Based on learnings, in this project button/select focus-visible styling is handled globally in
app/assets/main.css, and per-element utilities likefocus-visible:outline-accent/70should not be added.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/pages/about.vue (1)
62-62: Already flagged: remove inlinefocus-visible:outline-accent/70from the back button.This duplicates prior feedback; the project’s global button focus-visible style should be used instead.
Based on learnings, in the npmx.dev project button/select focus-visible styling is defined globally in
app/assets/main.css, and per-element utility focus styles should be avoided.app/components/About/LogoList.vue (1)
24-24:⚠️ Potential issue | 🟡 MinorReset list spacing in both logo
<ul>elements.
list-noneremoves bullets only; padding/margins can still cause indentation in prose layouts.Suggested fix
- <ul class="flex items-center flex-wrap gap-4 md:gap-x-6 md:gap-y-4 list-none"> + <ul class="flex items-center flex-wrap gap-4 md:gap-x-6 md:gap-y-4 list-none p-0 m-0"> ... - <ul class="flex items-center justify-center h-full gap-0.5 list-none"> + <ul class="flex items-center justify-center h-full gap-0.5 list-none p-0 m-0">Also applies to: 49-49
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
app/assets/logos/oss-partners/open-source-pledge-light.svgis excluded by!**/*.svgapp/assets/logos/sponsors/vercel-light.svgis excluded by!**/*.svgapp/assets/logos/sponsors/void-zero-light.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
app/assets/logos/oss-partners/index.tsapp/assets/logos/sponsors/index.tsapp/components/About/LogoImg.vueapp/components/About/LogoList.vueapp/pages/about.vue
✅ Files skipped from review due to trivial changes (1)
- app/assets/logos/sponsors/index.ts
| const props = defineProps<{ | ||
| src: | ||
| | string | ||
| | { | ||
| dark: string | ||
| light: string | ||
| } | ||
| }>() |
There was a problem hiding this comment.
Add alt support to AboutLogoImg and bind it on every <img>.
Right now the rendered logo images have no alt, so linked logos do not expose a reliable accessible name.
Suggested fix
<script setup lang="ts">
const props = defineProps<{
src:
| string
| {
dark: string
light: string
}
+ alt: string
}>()
</script>
<template>
<div v-if="typeof src === 'string'">
- <img :src="src" loading="lazy" height="36" class="w-auto block h-9" />
+ <img :src="src" :alt="alt" loading="lazy" height="36" class="w-auto block h-9" />
</div>
<div v-else-if="src.light === 'auto'">
<img
:src="src.dark"
+ :alt="alt"
loading="lazy"
height="36"
class="w-auto block light:invert light:grayscale h-9"
/>
</div>
<div v-else>
- <img :src="src.dark" loading="lazy" height="36" class="w-auto block light:hidden h-9" />
- <img :src="src.light" loading="lazy" height="36" class="w-auto block hidden light:block h-9" />
+ <img :src="src.dark" :alt="alt" loading="lazy" height="36" class="w-auto block light:hidden h-9" />
+ <img :src="src.light" :alt="alt" loading="lazy" height="36" class="w-auto block hidden light:block h-9" />
</div>
</template>Also applies to: 12-26
2 participants
🔗 Linked issue
Resolves #1623
🧭 Context
Adding Sponsors and OSS Partners sections
Screenshots
📚 Description
Added new sections. No major design changes yet, just using the approaches we already use. Also slightly adjusted the indents and styles on the page to make it more accessible and easier to use: