Add BMW M design system#579
Conversation
|
Hi @Sohaibcodecrafter! 🎉 Thanks for the contribution — this is a clean BMW M design system addition with good attention to the M tricolor branding and motorsport aesthetic. I will run a deep review and get back to you within 24h. Thanks for making open-design better! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36e8f25096
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,237 @@ | |||
| # Design System Inspired by BMW M | |||
There was a problem hiding this comment.
Adding design-systems/bmw-m/DESIGN.md creates a new curated design-system ID, but the locale coverage test enumerates every design-systems/*/DESIGN.md and expects each locale's ids.designSystems to exactly match that list (apps/web/tests/i18n/content.test.ts lines 89-99, built from designSystemSummaries plus designSystemIdsWithEnFallback in apps/web/src/i18n/content.ts lines 1047-1050). Because this commit doesn't add bmw-m to the localized summary/fallback ID lists, the i18n coverage check will fail for de/ru/fr when dependencies are available.
Useful? React with 👍 / 👎.
|
|
||
| ## 2. Color Palette & Roles | ||
| ### Brand & Accent | ||
| - **Primary** (`{colors.primary}` — #ffffff): The system's primary type and CTA color. Used for h1/h2/h3 display, body text on dark, and primary button labels (the buttons themselves are transparent or canvas-colored — the white text + outline IS the button). |
There was a problem hiding this comment.
These color bullets put a {colors...} token between the bold color name and the hex value, but the library swatch extractor in apps/daemon/src/design-systems.ts only recognizes Name: #hex or **Name** (#hex) shapes. With the current format, this new design system produces an empty swatches array in /api/design-systems, so the picker loses the BMW M color strip; use one of the extractor-supported forms or update the extractor before shipping this file.
Useful? React with 👍 / 👎.
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hey @Sohaibcodecrafter! 👋
This is a solid BMW M design system — excellent coverage of the motorsport aesthetic, dark canvas, and M tricolor signature. I found 4 substantive issues (P2 level) where the reasoning needs strengthening:
Key findings:
- Typography weight guidance conflicts with existing
design-systems/bmw/DESIGN.md(new doc says M display is 700, body 300 Light; existing BMW doc says display is 300 Light, body 400). - Dark-only claims are too absolute given configurator/account surfaces weren't analyzed.
- Several token/component references are undefined or misleading for agents.
- Accessibility guidance incomplete for white-on-photo text (no contrast strategy, scrim rules, or focus visibility).
See inline comments for details + concrete fixes. None are blockers — all are about making the doc more accurate for downstream agents.
Thanks for the contribution! 🚀
| ## 3. Typography Rules | ||
| ### Font Family | ||
| **BMW Type Next Latin** is BMW's licensed display + body typeface. The system uses two cuts: regular and Light. The fallback stack walks `-apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif`. | ||
|
|
There was a problem hiding this comment.
P2 Typography conflict with existing BMW system
This section says BMW M display is 700 and body is 300 Light. But design-systems/bmw/DESIGN.md documents BMW display as 300 Light and body/UI as regular 400, with nav emphasis at 900. Line 233 also admits weight-axis values beyond Light/regular aren't documented.
This creates contradictory guidance for agents working with BMW family systems.
Fix: Either add M-specific evidence for the 700/300 split, or soften this to observed examples and align fallback guidance with the existing BMW doc.
| > Category: Automotive | ||
| > Motorsport performance sub-brand. Near-black cockpit surfaces, BMW M tricolor accents, sharp engineering geometry. | ||
|
|
||
| ## 1. Visual Theme & Atmosphere |
There was a problem hiding this comment.
P2 Dark-only claims too absolute
The doc says "there is no light-mode marketing surface" and "footer never inverts," but also defines button-on-light and mentions configurator/account dialogs are light contexts. Lines 233 admit the configurator wasn't analyzed.
Turning an incomplete sample into universal rules is a reasoning-completeness issue.
Fix: Scope the black-canvas guidance to analyzed BMW M editorial/marketing pages and explicitly mark configurator/account/order flows as unresolved.
| ### Brand & Accent | ||
| - **Primary** (`{colors.primary}` — #ffffff): The system's primary type and CTA color. Used for h1/h2/h3 display, body text on dark, and primary button labels (the buttons themselves are transparent or canvas-colored — the white text + outline IS the button). | ||
| - **M Blue Light** (`{colors.m-blue-light}` — #0066b1): The first stop in the M tricolor stripe. Used on M-badge accents and motorsport chrome. | ||
| - **M Blue Dark** (`{colors.m-blue-dark}` — #1c69d4): The middle stop. The same hex as `{colors.bmw-blue}` — BMW's heritage corporate blue, repurposed as the middle band of the M stripe. |
There was a problem hiding this comment.
P2 Undefined token references
{colors.bmw-blue} is referenced but never defined in Section 2. {component.*} is used although components are prose headings rather than declared token objects. Line 234 says "Reference its YAML key" but this is a Markdown design system.
Agents may treat these as real resolvable tokens and generate invalid references.
Fix: Define every referenced token in the palette/component sections, remove undeclared aliases like {colors.bmw-blue}, and replace "YAML key" with "component name."
| > Motorsport performance sub-brand. Near-black cockpit surfaces, BMW M tricolor accents, sharp engineering geometry. | ||
|
|
||
| ## 1. Visual Theme & Atmosphere | ||
| BMW M's marketing surface is a near-pure black canvas (`{colors.canvas}` — #000) holding white BMW Type Next Latin headlines in **confident UPPERCASE**. The system has no decorative voltage of its own; brand energy comes from **full-bleed automotive photography** — cars cornering at speed, carbon-fiber wheel detail, driver cockpit shots, motorsport pit lanes — placed as edge-to-edge content that fills entire bands. UI chrome around the photography stays minimal: thin sans-serif copy, dividers as 1px hairlines (`{colors.hairline}`), all-caps button labels with no fill until hovered. |
There was a problem hiding this comment.
P2 Accessibility gaps for white-on-photo text
The file requires white type over full-bleed photography but bans gradient backdrops and gives no contrast strategy beyond crop placement. This risks unreadable hero text on bright vehicle/track imagery.
The "48 × 48 meets WCAG AAA" claim (line 198) only covers target size, not contrast, focus visibility, or text-over-image failure modes.
Fix: Add rules for minimum contrast, allowed scrims/solid text panels when crops fail, focus-ring colors on black/photo backgrounds, and measurable tap-target requirements for text-only tabs.
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@Sohaibcodecrafter Thanks for adding the BMW M design system — the DESIGN.md structure and brand coverage are thoughtful and the new file is easy to follow. I found one integration blocker before this can merge: the new curated design-system ID also needs localized coverage so the existing i18n coverage checks stay green. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode| @@ -0,0 +1,237 @@ | |||
| # Design System Inspired by BMW M | |||
There was a problem hiding this comment.
Blocking: adding this new design-systems/bmw-m/DESIGN.md directory creates a curated design-system ID, but the PR does not update localized design-system coverage. The existing apps/web/tests/i18n/content.test.ts enumerates every design-systems/*/DESIGN.md and expects each locale's LOCALIZED_CONTENT_IDS[locale].designSystems to match that list; those IDs are built from designSystemSummaries plus designSystemIdsWithEnFallback in apps/web/src/i18n/content.ts. Because bmw-m is not in any of the de/ru/fr summary or fallback lists, that coverage test will fail once dependencies are installed. Please add bmw-m to the localized fallback ID arrays (or provide localized summaries) for all supported locales alongside this new DESIGN.md.
There was a problem hiding this comment.
i have uodated according to the feed back
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@Sohaibcodecrafter Thanks for the follow-up fixes here — the localized fallback IDs are now covered and the BMW M guidance is much more carefully scoped. I found one remaining non-blocking integration issue around how the palette is parsed for the picker. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode|
|
||
| ## 2. Color Palette & Roles | ||
| ### Brand & Accent | ||
| - **Primary** (`{colors.primary}` — #ffffff): The system's primary type and CTA color. Used for h1/h2/h3 display, body text on dark, and primary button labels (the buttons themselves are transparent or canvas-colored — the white text + outline IS the button). |
There was a problem hiding this comment.
The color bullets still use the **Name** ({colors...} — #hex) shape, which the current swatch extractor in apps/daemon/src/design-systems.ts does not recognize. That extractor only collects Name: #hex or **Name** (#hex)-style entries, so this new design system currently yields an empty swatches array and the design-system picker loses the BMW M color strip. Please either reformat these bullets to an extractor-supported shape, e.g. - **Primary** (#ffffff): ..., or update the extractor/tests to support token references before the hex.
|
Thanks for the quick fixes, @Sohaibcodecrafter! 🙌 Verified the i18n coverage is now complete — mrcfps's blocking finding is resolved. Looking good! |
|
Hi @Sohaibcodecrafter! The PR is currently blocked because the earlier Two paths forward:
Note: @lefarcen (the account you tagged) is actually a bot — it cannot merge PRs. The maintainers will see this PR when it's ready for their review. Thanks for your patience! 🙏 |
mrcfps
left a comment
mrcfps
left a comment
There was a problem hiding this comment.
@Sohaibcodecrafter Thanks for the quick follow-up fixes here — the localized fallback coverage is now in place, and the palette format now matches the extractor shape. I found one remaining non-blocking picker-quality issue: the extracted accent will still be white rather than one of the M tricolor brand colors. 🙏
Generated by Looper 0.5.6 · runner=reviewer · agent=opencode|
|
||
| ## 2. Color Palette & Roles | ||
| ### Brand & Accent | ||
| - **Primary** (#ffffff): `{colors.primary}`. The system's primary type and CTA color. Used for h1/h2/h3 display, body text on dark, and primary button labels (the buttons themselves are transparent or canvas-colored — the white text + outline IS the button). |
There was a problem hiding this comment.
The swatch parser can now read these bullets, but the picker heuristic will still choose white as the accent for this design system. In apps/daemon/src/design-systems.ts, extractSwatches picks the accent by looking for names containing primary brand, brand primary, accent, brand, then primary; because this changed line names #ffffff as Primary and the M tricolor lines below are named only M Blue Light, M Blue Dark, and M Red, the returned swatch row becomes mostly black/gray/white instead of showing the BMW M identity colors. That keeps the design-system picker from displaying the intended M color strip even though parsing no longer returns an empty array. Please either avoid naming the white text color Primary (for example On Dark) or include Brand/Accent in at least one M tricolor color name, such as Brand M Blue Light, so the extracted accent is one of the BMW M colors.
|
Nice fix @Sohaibcodecrafter! The i18n coverage is now complete - you've added The palette format issue @mrcfps mentioned (color bullet format that the extractor doesn't recognize) is still present, but that's non-blocking per mrcfps's review. The 4 P2 documentation issues from my earlier review also remain (typography weight description, dark-only claims, undefined token refs, accessibility gaps). Those are non-blocking as well, but worth addressing for clarity. Current state: i18n blocker resolved ✅, non-blocking feedback still open for iteration. |
|
Excellent work @Sohaibcodecrafter! 🎉 All issues have been resolved:
The PR is ready for maintainer approval. The CHANGES_REQUESTED status from the earlier review will be cleared during the approval process. Thanks for the thorough iteration and for contributing the BMW M design system to open-design! |
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hi @Sohaibcodecrafter! All the blocking issues have been addressed:
✅ i18n coverage — bmw-m is now in all localized fallback ID arrays
✅ Palette format — Color bullets now use the extractor-supported - **Name** (#hex): shape
✅ P2 documentation findings — All prior suggestions addressed
@mrcfps's latest review confirms the integration blockers are resolved. The remaining picker-quality issue is non-blocking.
Looks good to me; deferring final approval to a maintainer. 🎉
3 participants
Adds BMW M design system under design-systems/bmw-m.