Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds dark mode styling, switches from self-hosted Raleway font files to Google Fonts, inlines the CakePHP logo as an SVG for theme-based color adaption, and updates spacing and color utility classes to support a dark theme.
- Introduces dark mode classes and custom theme color tokens.
- Replaces raster/logo image with inline SVG but omits accessible text.
- Removes locally hosted font files and license, adds Google Fonts inclusion only on the home page.
Reviewed Changes
Copilot reviewed 3 out of 13 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| webroot/font/Raleway-License.txt | Removes bundled Raleway license along with self-hosted font assets. |
| templates/Pages/home.php | Adds Google Fonts links, dark mode classes, inline SVG logo, spacing and color adjustments. |
| resources/css/style.css | Removes self-hosted @font-face blocks; introduces theme tokens, link styling, and utility-based color usage. |
Comments suppressed due to low confidence (1)
templates/Pages/home.php:1
- Replacing the img (which had alt text) with an inline SVG removed its accessible name; add a <title> element with an id and aria-labelledby, or set role='img' and aria-label='CakePHP' on the so assistive technologies announce it properly.
<?php
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| CakePHP: the rapid development PHP framework: | ||
| <?= $this->fetch('title') ?> | ||
| </title> | ||
| <link href="https://fonts.googleapis.com/css2?family=Lexend:wght@100..900&family=Raleway:ital,wght@0,100..900;1,100..900&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
We purposely added the fonts in the repo to not be in violation of GDPR and include external google services without user consent.
There was a problem hiding this comment.
Yeah, thought there was a reasoning behind this.
What about using https://fonts.bunny.net from Bunny. This a privacy first font CDN service.
As I really don't like all those extra assets in the skeleton app.
There was a problem hiding this comment.
Updated and swapped to to use bunnycdn
There was a problem hiding this comment.
cool, didn't know that one existed. As long as that font provider doesn't get nuked in the future this is fine by me 😁
There was a problem hiding this comment.
That seems arbitrary. So bunny.net can host a font but google cannot?
Use more generic 'font-heading' class
I strongly disagree with this. This hinders offline prototyping, educational workshops etc. |
Then it falls back to the default font which also looks nice. We are talking about an on-boarding page here. This should have minimal footprint. |
| /* Styling for default app homepage */ | ||
| a { | ||
| @apply text-link hover:text-sky-600 transition-all underline; | ||
| } |
There was a problem hiding this comment.
I usually add default styling for all the headings as well so one doesn't have to add all the text-xl etc. stuff to each h1 -h6 every time
There was a problem hiding this comment.
i saw @markstory already added arbitrary variants for this. Should we adjust?
There was a problem hiding this comment.
I added utility classes as I was under the impression that was how one used tailwind 🫠 I'm fine with element selectors too.
What do you mean by minimal footprint for an onboarding page? |
More often than not all those extra assets get committed and not used, it would be great if we could contain all those unnecessary assets on the page where they are used. This means: not including font assets and not including image assets. Icons can be swapped in favor of SVG's
|
| /* Styling for default app homepage */ | ||
| a { | ||
| @apply text-link hover:text-sky-600 transition-all underline; | ||
| } |
There was a problem hiding this comment.
I added utility classes as I was under the impression that was how one used tailwind 🫠 I'm fine with element selectors too.
|
As discussed on discord I consolidated on removing the heading fonts as these are only used on the welcome page. |
Added
extra:
googlebunnycdn (privact first) raleway font,. My take here is that we don't ship unnecessary assets that in many cases are dropped from the end project.For reference: this is the screenshot from the previous PR