Add support for crowns as a charge#9
Conversation
| 'dovetailed', | ||
| 'ducal', | ||
| 'embattled', | ||
| ]; |
There was a problem hiding this comment.
missing a as const as the end of this declaration
There was a problem hiding this comment.
Thank you very much, at runtime it works perfectly :)
But I didn't setup a CI (yet ?) , so I have to run manually npm run test, which is showing me some errors:
- can you run prettier on all committed code ? (
prettier --write src/) - can you fix the type errors (can be seen with
npm run typecheck)
you will need to add in arbitraries.ts increateChargeArbsomething like
else if (chargeName === 'crown') {
return fc.record<Crown>({
name: fc.constant(chargeName),
tincture: tinctureArb,
countAndDisposition: countAndDistionArb,
type: fc.constantFrom(...crownTypes),
});
}
Aside from that, It's good for me. When fixed, I'll merge.
Thank you again!
|
Thanks for the review. I have two questions about that:
|
|
It's a good question, as it will make the change far more complicated:
Here the quick fix could to just not support ducal in the first version, as the parser will need some tweak to be able to support the 2 possible positions A plugin system is definitely something we could do, but it will take quite a lot a time to implement , so don't expect it very soon... |
I could start working on it, if that's OK with you. |
|
It's open source, you don't need my permission to work on it :) |
Images come from http://www.vikinganswerlady.com/Stars/Heraldry_SVG_Images/index.htm