Eop card browser/m4 eop styling config - #3315
Conversation
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
… removed 'opaque' tags Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
|
Hi @rewtd, @sydseter What's done
What's configurable now
what else should be configurable?Per-image positioning (Didn't need it since the images fit well without handling each card image positioning, but that's not guaranteed for future decks.) What do you think? |
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
This sounds good. I'll have a look |
|
@ayman-art have a look at the review comments from qltysh. |
There was a problem hiding this comment.
🟡 Not ready to approve
The new EopCard component can render with an undefined card (and calls eopValue with possibly undefined inputs), which can lead to incorrect output/TypeScript errors and should be guarded/fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds YAML-backed styling and per-card image configuration for the EoP (Elevation of Privilege) edition in the Svelte site, and wires that configuration through layout data into the card preview/browser UI.
Changes:
- Added new
source/YAML files for EoP suit styling (colors) and card image paths (per card ID). - Introduced
CardImagesServiceandSuitStylingService(with Vitest coverage) and exposed their data via+layout.server.ts. - Refactored EoP rendering in
CardPreviewinto a dedicatedEopCardcomponent and passedcardImages/suitStylingthrough relevant routes/components.
File summaries
| File | Description |
|---|---|
| source/eop-styling-5.0.yaml | Adds suit color configuration for EoP 5.0. |
| source/eop-card-images-5.0.yaml | Adds per-card image path mapping for EoP 5.0. |
| cornucopia.owasp.org/src/routes/edition/[edition]/+page.svelte | Passes card image + styling data into CardPreview for edition view. |
| cornucopia.owasp.org/src/routes/edition/[edition]/[card]/+page.svelte | Passes card image + styling data into CardFound. |
| cornucopia.owasp.org/src/routes/edition/[edition]/[card]/[version]/+page.svelte | Passes card image + styling data into CardFound. |
| cornucopia.owasp.org/src/routes/edition/[edition]/[card]/[version]/[lang]/+page.svelte | Passes card image + styling data into CardFound. |
| cornucopia.owasp.org/src/routes/cards/+page.svelte | Passes card image + styling data into CardPreview for cards index. |
| cornucopia.owasp.org/src/routes/cards/[card]/+page.svelte | Passes card image + styling data into CardFound for card detail. |
| cornucopia.owasp.org/src/routes/+layout.server.ts | Loads EoP card images + suit styling in the root layout. |
| cornucopia.owasp.org/src/lib/services/suitStylingService.ts | New service to load and cache suit styling YAML. |
| cornucopia.owasp.org/src/lib/services/suitStylingService.test.ts | Tests for suit styling loader behavior. |
| cornucopia.owasp.org/src/lib/services/cardImagesService.ts | New service to load and cache card images YAML. |
| cornucopia.owasp.org/src/lib/services/cardImagesService.test.ts | Tests for card images loader behavior. |
| cornucopia.owasp.org/src/lib/components/eopCard.svelte | New EoP card rendering component using styling + images. |
| cornucopia.owasp.org/src/lib/components/eopCard.css | Extracted CSS for the EoP card renderer. |
| cornucopia.owasp.org/src/lib/components/cardPreview.svelte | Uses EopCard instead of inline EoP markup/styles; adds props for images/styling. |
| cornucopia.owasp.org/src/lib/components/cardFound.svelte | Plumbs image/styling props into CardBrowser. |
| cornucopia.owasp.org/src/lib/components/cardBrowser.svelte | Plumbs image/styling props into CardPreview. |
Review details
- Files reviewed: 18/26 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| <div | ||
| class="card-render eop {card?.suit} n{eopValue(card?.value)}" | ||
| style={suitStyle ? `--eop-tab:${suitStyle.tab}; --eop-watermark:${suitStyle.watermark}; --eop-royal:${suitStyle.royal};` : ''} | ||
| > | ||
| {#if !['J', 'Q', 'K'].includes(card?.value)} | ||
| <span class="watermark" aria-hidden="true">{eopValue(card?.value)}</span> | ||
| {/if} | ||
| <div class="artwork" style={cardImage ? `background-image:url(${cardImage.image});` : ''}></div> | ||
| <div class="number-tab"><span>{eopValue(card?.value)}</span></div> | ||
| <div class="text-block"> | ||
| <p class="suit-name">{card?.suitName}</p> | ||
| <p class="description">{card?.desc}</p> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Is this generated? Could this be generalised to be applicable to the entire deck instead of being a separate component?
There was a problem hiding this comment.
This was originally inline in cardPreview.svelte, handling the EoP branch alongside the default layout. Since EoP's layout (artwork + watermark + tab) is structurally different, the idea was to extract it into its own file instead of growing cardPreview.svelte. Do you think this is redundant work? Would you rather it stayed inline?
There was a problem hiding this comment.
It's not really an improvement. If you create specific ts files whose only purpose is to support the edition, then you make the coupling between code and data stronger. You need to generalise the code so that it can be used for any of the editions. The code can't really refer to the data if you want to add new editions without having to change the code.
There was a problem hiding this comment.
Thanks for clarifying! Yes, I want to make sure there's a decoupling between code and data. I merged eopCard.svelte back into cardPreview.svelte, and replaced the card.edition === 'eop' check with a generalized one:
let editionCardImages = $derived(cardImages?.[card?.edition ?? '']);{#if card && editionCardImages}It now picks the layout by checking if image data exists for that edition, not by its name. I also removed the hardcoded eop class/CSS variables so nothing in the logic refers to that specific edition anymore.
I also switched the CSS import to:
import.meta.glob('./*Card.css', { eager: true }); // example: eopCard.cssThis automatically includes any new deck's CSS, no manual import needed for future decks.
So adding a new deck now only requires:
{edition}-styling-{version}.yaml-> suit colors{edition}-card-images-{version}.yaml-> card images{edition}Card.css(its own layout styling) -> automatically imported using this new import line.
No changes to cardPreview.svelte or any other code file needed!
Would like to know your thoughts about this!
There was a problem hiding this comment.
This is great. Yes, perfect.
There was a problem hiding this comment.
Make sure to the capabilities are documented in the readme.
There was a problem hiding this comment.
README is updated now.
| fallbackTranslation: event.locals.fallbackTranslation, | ||
| lang: event.locals.lang | ||
| lang: event.locals.lang, | ||
| cardImages: (new CardImagesService()).getCardImages('eop', eopVersion), |
There was a problem hiding this comment.
Any reason you need to fetch the EOP card images here? Could this be generalised so that it fetches all card images for all decks and not just for EOP?
There was a problem hiding this comment.
Good point! A solution for this will be to make layout.server.ts loops over DeckService.getLatestEditions() instead of hardcoding 'eop', so any deck's yaml gets loaded automatically once it exists, and decks with no images are not affected.
There was a problem hiding this comment.
Note: We should keep in mind future decks might need a different styling/image structure than EoP's, so the SuitStyling/CardImage types may need to grow beyond tab/watermark/royal or to be loosened.
There was a problem hiding this comment.
Sure, but this is a future improvement and perfectly fine to leave for later.
|
@ayman-art have a look at mine and Copilot's comments as well. |
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
…ub.com/OWASP/cornucopia into eop-card-browser/m4-eop-styling-config
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
…ub.com/OWASP/cornucopia into eop-card-browser/m4-eop-styling-config
…-hit and parse-failure tests to increase the coverage. Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Thanks for your review. I have addressed yours, Copiot's, and qlty's comments. Ready for another look! |
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new YAML loader currently caches missing-root-key results silently (masking config errors), and the root layout now ships full image/styling maps to every route which has avoidable payload/build-size impact.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
cornucopia.owasp.org/src/lib/services/yamlDataLoader.ts:25
- Building the YAML path via string interpolation hard-codes POSIX separators and bypasses
pathnormalization. Usingpath.join(...)avoids cross-platform path issues and makes the intent clearer.
const file = `${__dirname}/../source/${edition}-${this.fileSuffix}-${version}.yaml`;
cornucopia.owasp.org/src/lib/services/yamlDataLoader.ts:34
yaml.load(...)is cast asRecord<string, T>, but if the YAML is valid yet missing the expected root key (e.g.suits/cards),databecomesundefinedand is still cached, silently masking configuration errors and preventing a later retry. This should validate the parsed structure and only cache when the expected key exists.
const yamlData = fs.readFileSync(file, 'utf8');
const parsed = yaml.load(yamlData, { schema: yaml.FAILSAFE_SCHEMA }) as Record<string, T>;
const data = parsed[this.rootKey];
this.cache.push({ edition, version, data });
return data;
cornucopia.owasp.org/src/routes/+layout.server.ts:43
+layout.server.tsruns for every page, and its returned data is serialized to the client (and duplicated into each prerendered route). Returning the fullcardImages/suitStylingmaps here will increase payload and build output size even on pages that never render cards; consider moving this to a nested+layout.server.tsunder/cardsand/edition(or a dedicated endpoint) so only card pages pay the cost.
fallbackTranslation: event.locals.fallbackTranslation,
lang: event.locals.lang,
cardImages: getCardImagesByEdition(),
suitStyling: getSuitStylingByEdition()
- Files reviewed: 18/26 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
3e62bda to
c4fa548
Compare
Signed-off-by: Ayman Algamal <ayman01287985950@gmail.com>
Build artifacts:
Translation Check ReportThe following sentences/tags have issues in the translations: SpanishFile: Untranslated TagsThe following tags have identical text to English (not translated): T00105, T00140 RussianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00001, T00003, T00004, T00090, T00100, T00105, T00210, T00220, T00230, T00240, T00250, T00260, T00270, T00280, T00290, T00300, T00310, T00320, T00330, T00340, T00350, T00360, T00370, T00380, T00390, T00400, T00410, T00420, T00430, T00440, T00450, T00460, T00470, T00480 RussianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00020, T00120, T00130, T00220, T00240, T00310, T00311, T00320, T00330, T00340, T00350, T00360, T00370, T00380, T00390, T00400, T00510, T00520, T00530, T00610, T01010, T01070, T01160, T01170, T01180, T01200, T01210, T01220, T01301, T01411, T02680, T02690, T02700, T02710, T02720, T02730, T02780, T03010 HindiFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090 RussianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00020, T00090, T00120, T00130, T00220, T00240, T00310, T00311, T00320, T00330, T00340, T00350, T00360, T00370, T00380, T00390, T00400, T00510, T00520, T00530, T00610, T01010, T01070, T01160, T01170, T01180, T01200, T01210, T01220, T01301, T01411, T02680, T02690, T02700, T02710, T02720, T02730, T02780, T03010 ukFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090 SpanishFile: Missing TagsThe following tags are present in the English version but missing in this translation: T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00020, T00380, T01590, T02940, T03140, T03160, T03180, T03200, T03210, T03220, T03230, T03250, T03270, T03280, T03290, T03300, T03310, T03320, T03330, T03340, T03360, T03370, T03380, T03390, T03400, T03410, T03430, T03440, T03450, T03460, T03480, T03500, T03510, T03520, T03530, T03550, T03560, T03570, T03590, T03600, T03610, T03620, T03630, T03640, T03650, T03660, T03670, T03680, T03690, T03700, T03720, T03771, T03773, T03775, T03800, T03810, T03820, T03830, T03840, T03850, T03860, T03870, T03900, T03940, T03950 FrenchFile: Missing TagsThe following tags are present in the English version but missing in this translation: T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00200, T01100, T03110, T03120, T03771, T03773, T03775 HungarianFile: Missing TagsThe following tags are present in the English version but missing in this translation: T00005, T00161, T00162, T01301, T01311, T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00020, T00140, T00145, T00200, T00210, T00220, T00230, T00240, T00300, T00320, T00340, T00350, T00360, T00370, T00380, T00390, T00400, T00500, T00510, T00520, T00600, T00610, T00700, T00710, T00720, T00730, T00740, T00750, T00760, T00770, T00780, T00790, T00800, T00810, T00830, T00840, T00900, T00910, T00920, T01000, T01020, T01060, T01100, T01110, T01120, T01130, T01140, T01150, T01160, T01170, T01190, T01200, T01240, T01250, T01260, T01270, T01280, T01290, T01300, T01400, T01410, T01420, T01430, T01431, T01440, T01450, T01500, T01510, T01520, T01530, T01540, T01550, T01560, T01570, T01571, T01580, T01590, T01600, T01610, T01700, T01710, T01720, T01730, T01740, T01800, T01810, T01811, T01820, T01900, T01910, T01920, T01930, T01940, T01960, T01970, T01980, T02000, T02010, T02020, T02030, T02040, T02100, T02120, T02140, T02200, T02220, T02240, T02250, T02260, T02280, T02290, T02300, T02310, T02320, T02340, T02400, T02410, T02420, T02440, T02450, T02460, T02480, T02490, T02500, T02510, T02520, T02540, T02600, T02610, T02620, T02630, T02650, T02680, T02690, T02700, T02710, T02720, T02730, T02760, T02770, T02790, T02800, T02810, T02820, T02840, T02850, T02860, T02870, T02880, T02890, T02900, T02910, T02920, T02930, T02940, T02950, T02960, T02970, T02980, T02990, T03000, T03020, T03100, T03110, T03120, T03140, T03160, T03200, T03210, T03220, T03230, T03250, T03270, T03280, T03290, T03300, T03310, T03320, T03330, T03340, T03360, T03370, T03380, T03390, T03400, T03410, T03430, T03450, T03460, T03480, T03500, T03510, T03520, T03530, T03550, T03560, T03570, T03590, T03600, T03610, T03620, T03630, T03640, T03650, T03660, T03670, T03680, T03690, T03700, T03720, T03740, T03760, T03771, T03773, T03775, T03800, T03810, T03820, T03830, T03840, T03900, T03920, T03950 ItalianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00380, T02940, T03250, T03771, T03773, T03775 DutchFile: Missing TagsThe following tags are present in the English version but missing in this translation: T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00500, T03771, T03773, T03775 NorwegianFile: Missing TagsThe following tags are present in the English version but missing in this translation: T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00380, T01700, T03140, T03160, T03180, T03200, T03210, T03220, T03230, T03250, T03270, T03280, T03290, T03300, T03310, T03320, T03330, T03340, T03360, T03370, T03380, T03390, T03400, T03410, T03430, T03440, T03450, T03460, T03480, T03500, T03510, T03520, T03530, T03550, T03560, T03570, T03590, T03600, T03610, T03620, T03630, T03640, T03650, T03660, T03670, T03680, T03690, T03700, T03771, T03773, T03775 Portuguese (Brazil)File: Missing TagsThe following tags are present in the English version but missing in this translation: T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00380, T02250, T02290, T02310, T02450, T02490, T02510, T03100, T03110, T03120, T03140, T03160, T03180, T03200, T03210, T03220, T03230, T03250, T03270, T03280, T03290, T03300, T03310, T03320, T03330, T03340, T03360, T03370, T03380, T03390, T03400, T03410, T03430, T03440, T03450, T03460, T03480, T03500, T03510, T03520, T03530, T03550, T03560, T03570, T03590, T03600, T03610, T03620, T03630, T03640, T03650, T03660, T03670, T03680, T03690, T03700, T03720, T03771, T03773, T03775 Portuguese (Portugal)File: Untranslated TagsThe following tags have identical text to English (not translated): T00380, T03771, T03773, T03775 RussianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00380, T01411, T03771, T03773, T03775 SpanishFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00380, T02940 FrenchFile: Missing TagsThe following tags are present in the English version but missing in this translation: T01411 Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00200, T01100, T03110, T03120 HindiFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090 ItalianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00380, T02940, T03250 DutchFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00380, T02270, T02290, T03250 NorwegianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00380, T01700, T03140, T03160, T03180, T03200, T03210, T03220, T03230, T03250, T03270, T03280, T03290, T03300, T03310, T03320, T03330, T03340, T03360, T03370, T03380, T03390, T03400, T03410, T03430, T03440, T03450, T03460, T03480, T03500, T03510, T03520, T03530, T03550, T03560, T03570, T03590, T03600, T03610, T03620, T03630, T03640, T03650, T03660, T03670, T03680, T03690, T03700 Portuguese (Brazil)File: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00330, T00340, T00350, T00360, T00370, T00380, T02240, T02260, T02280, T02300, T02320, T02340, T02440, T02460, T02480, T02500, T02520, T02540 Portuguese (Portugal)File: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00380 RussianFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090, T00380 ukFile: Untranslated TagsThe following tags have identical text to English (not translated): T00090 |
Description
SuitStylingService,CardImagesService).EopCardcomponent that draws EoP cards with their own look (background artwork, watermark number, color tab), since EoP cards look different from the other decks' cards. This component is now used on every page that shows a card (/cards,/cards/[card],/edition/[edition]/...).Changes
Feature 1: EoP suit styling and card images now come from YAML files
source/eop-styling-5.0.yaml: suit colors (tab/watermark/royal)source/eop-card-images-5.0.yaml: image path for each cardsrc/lib/services/suitStylingService.ts/src/lib/services/cardImagesService.ts: read and cache this datasrc/routes/+layout.server.ts: loads this data for every page. It now checks every deck, not just EoP, for a matching yaml file, so a future deck can add its own file with no code changes needed. Decks without a file (all except EoP today) are simply skipped, no error.src/lib/services/yamlDataLoader.ts: shared code both services now use, since they were doing almost the same thing (flagged by qlty as duplicate code)Feature 2: Card style picked automatically in cardPreview.svelte
src/lib/components/cardPreview.svelte: draws EoP's card look (image, watermark, colored tab, royal color) directly, instead of using a separateEopCardfile. To decouple the code from data, it checks if there's image data for that deck instead of checking if the deck is named'eop'. This means a future deck with the same structure needs no code change here.src/lib/components/eopCard.css: EoP's own styling (sizes, colors) for this card look. No longer imported by hand.cardPreview.svelteloads it automatically usingimport.meta.glob('./*Card.css', { eager: true }), so any{edition}Card.cssfile is imported on its own.src/lib/components/cardBrowser.svelte,cardFound.svelte: pass the card images/colors down tocardPreview.svelteFeature 3: Card image fixes
opaquetagsFeature 4: Tests
src/lib/services/cardImagesService.test.ts/suitStylingService.test.ts: added tests for cache-hit and parse-failure cases, removed a redundantasyncfrom two synchronous test casesNotes
SuitStyling/CardImageare still fixed to EoP's fields (tab/watermark/royal). A future deck with different needs will require loosening these types.Resolved or fixed issue: refactoring related to Adding a endpoint for each eop card and to the card browser #1322
AI Tool Disclosure
[e.g. GitHub CoPilot, ChatGPT, JetBrains Junie etc.][e.g. GPT-4.1, Claude Haiku 4.5, Gemini 2.5 Pro etc.][Summarize the key prompts or instructions given to the AI tools]Affirmation