Skip to content

Adding LEGENDS expansion #65

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

lilasquared
Copy link

@lilasquared lilasquared commented Mar 7, 2022

WIP

Wanted to get initial feedback on my attempt to start incorporating the LEGENDS expansion cards. you should be able to preview the cards on the /test/ui page. I tweaked the color of DESTINATION cards because the LEGENDS cards use a red color to indicate which card can be used to play them. Its just s slightly darker shade of the burgundy.

Screen Shot 2022-03-07 at 5 54 57 PM

Everdell_Collector's_Rule_Sheet_v2.pdf

TODO LIST:

  • create legend card definitions
  • update game options to allow legends expansion
  • deal cards (1 legendary critter and 1 legendary construction) to each player at game start
  • modify player UI to show legend cards in hand (similar to adornments)
  • Add ability to play legend card by "upgrading" the related critter / construction (untested)
  • Implement legend card functionality for pre-checks, placement effects, etc
  • touch up legend card descriptions
  • add ability for legend cards to increase size of city. Each legend card (once played) adds 1 space to the player's city
  • Set up testing scenarios with legends
  • prevent placing cards that have been upgraded. (ie: if you have mcgregors market played, you are no longer allowed to play more farms)

CARD EFFECT TODO LIST:

  • amilla glistendew
  • bridge of the sky
  • cirrus windfall
  • foresight
  • fynn nobletail
  • mcgregors market
  • oleanders opera house
  • poe
  • silver scale spring
  • the green acorn

@vercel
Copy link

vercel bot commented Mar 7, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @ymichael on Vercel.

@ymichael first needs to authorize it.

@lilasquared lilasquared marked this pull request as draft March 7, 2022 22:51
},
playInner: (gameState: GameState, gameInput: GameInput) => {
// TODO: Implement this
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also put a pointsInner placeholder, since you'll need to calculate points based on stored construction

Copy link
Owner

@ymichael ymichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey I took a pass on the PR so far and it looks great so far!

Some quick thoughts:

  1. I think we can leave the UI tweaking till last (eg. colors and how the cards look) it does look like some of the legends cards have very long names so we might need to change how they render anyway. @elynnlee might have some thoughts here too!
  2. Can we update the card names and their exports to be camelCased eg. cards/legends/silverScaleSpring.ts
  3. For each of the card files, we might want to consider using a default export instead since that's the only thing we're exporting
  4. I'm not sure I understand the purpose of src/pages/api/create-game-from-state.ts but if it is temporary for your testing that's cool. We have https://everdell.vercel.app/test/game as an example test page that is able to create a game the additional route

Looking forward to the rest of the PR!

@@ -1 +1 @@

{}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm was this giving your issues? .prettierrc can be either yaml OR json so I'm curious why the blank file didn't work for you 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this was actually, I'm not sure why. I am using vscode and it wouldn't format unless i had something here. maybe i can take a look at why or remove this once the PR is actually done

Copy link

@sorahn sorahn Mar 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why.

It also doesn't work with the .yml or .yaml extension if completely empty.

If you have the prettier extension in VSCode (not sure about others) to only run if there is a .prettier config file, there must be at least something to tell it that the config exists.

I work in repositories that are set up with and without prettier, so unfortunately I cannot just tell it always run even though I would much prefer that it does.

@elynnlee
Copy link
Collaborator

elynnlee commented Mar 8, 2022

+1! Excited to see this, and thank you so much for putting this together 🙌!! I left a few small notes on the copy on the cards, but agree w/ @ymichael that we can adjust the UI a little later + need to generally think through how to handle the longer names for some of these cards.

@lilasquared
Copy link
Author

  1. I can leave some of that to the end, I already did a little more tweaking to adjust the size but looking holistically at the end is definitely worthwhile
  2. sure, no prob
  3. named exports have many advantages over default, I can link some write ups for you but generally named is preferred
  4. I added a few testing things so will remove them once its good to go.

Generally I might need help understanding how to implement some of the actual game logic for the cards, its a bit hairy because they have new mechanics like "play an event for free" or "put this card on top". while not physically on top there needs to be some way to link the two. I can send you resources for the expansion rules if that helps too

@lilasquared
Copy link
Author

Also, each card adds 1 to the amount of cards allowed in your city. (different from the wanderer who simple doesn't count) so any thoughts on how to accomplish that would be nice too :)

@ymichael
Copy link
Owner

ymichael commented Mar 8, 2022

named exports have many advantages over default, I can link some write ups for you but generally named is preferred

This would be great, thanks!

I can send you resources for the expansion rules if that helps too

Yea this would be super helpful since we're not familiar with this expansion

Also, each card adds 1 to the amount of cards allowed in your city. (different from the wanderer who simple doesn't count) so any thoughts on how to accomplish that would be nice too :)

For this, we probably need to do something similar to Player#maxHandSize where the max hand size is dynamically computed as opposed to just a global MAX_CITY_SIZE constant

@elynnlee
Copy link
Collaborator

elynnlee commented Mar 8, 2022

Generally I might need help understanding how to implement some of the actual game logic for the cards, its a bit hairy because they have new mechanics like "play an event for free" or "put this card on top". while not physically on top there needs to be some way to link the two.

For "Put this card on top", is this similar to how the Dungeon works? If so, you can look in types.ts to find PlayedCardInfo -- there's a pairedCards array that you can use to store cards that are on top of that card. Let me know if that helps!

For "play an event for free", do you have to spend a worker? Or do you get it truly for free (no resources, no workers, etc)? Either way, I think you'll need to update the canPlayCheck to account for this. Happy to help with this part if that's helpful!

I can send you resources for the expansion rules if that helps too

This would be super helpful!! That might help us give you better advice too 😄

@lilasquared
Copy link
Author

I updated the PR description with a link to the rulebook from the collectors edition, which is the only place I know to find the expansion.

For this, we probably need to do something similar to Player#maxHandSize where the max hand size is dynamically computed as opposed to just a global MAX_CITY_SIZE constant

Will take a look, ty!

For "Put this card on top", is this similar to how the Dungeon works? If so, you can look in types.ts to find PlayedCardInfo -- there's a pairedCards array that you can use to store cards that are on top of that card. Let me know if that helps!

I will check that out. It's similar to the dungeon except the cards are still considered to be part of your city. They count for points at the end, activate for production, etc etc.

For "play an event for free", do you have to spend a worker? Or do you get it truly for free (no resources, no workers, etc)? Either way, I think you'll need to update the canPlayCheck to account for this. Happy to help with this part if that's helpful!

It is the destination reward for amilla glistendew, so you do place a worker

@lilasquared
Copy link
Author

there's another expansion set of regular cards called "Extra! Extra!" which i plan to do at some point as well, but i don't know where to find those rules that aren't in swedish :P

@elynnlee
Copy link
Collaborator

elynnlee commented Mar 8, 2022

I will check that out. It's similar to the dungeon except the cards are still considered to be part of your city. They count for points at the end, activate for production, etc etc.

Oh cool! Take a look at payForCard within player.ts. Starting around line 1188, there's a section around how to pay using the Dungeon. You'll want to something similar, except you don't want to remove it from the city.

It is the destination reward for amilla glistendew, so you do place a worker

Oh gotcha! Yea, this should be workable -- you can see handleClaimEventGameInput for how claiming an event works. You shouldn't need to check whether the player has enough workers, has met requirements, etc since that's what the card allows you to do.

Hopefully these help! I wrote this up quick, but def feel free to ask more questions :)

@lilasquared
Copy link
Author

@ymichael this is one such article that goes through the differences of each and the benefits of one over the other.

https://www.bundleapps.io/blog/use-named-exports-over-default-exports-in-javascript

highlights:

  • named are explicit vs implicit (i can import foo from './someThingNotNamedFoo')
  • refactoring is easier, because the same name MUST be used everywhere
  • lookup is easier (same reason as above)
  • tree shaking is better (probably not relevant here)

happy to undo it but typically i just stick to named everywhere in my projects ¯_(ツ)_/¯

@elynnlee
Copy link
Collaborator

elynnlee commented Mar 8, 2022

there's another expansion set of regular cards called "Extra! Extra!" which i plan to do at some point as well, but i don't know where to find those rules that aren't in swedish :P

brb learning swedish

haha just kidding -- I wonder where we can find these!! So ❤️ about your enthusiasm!!

@ymichael
Copy link
Owner

ymichael commented Mar 8, 2022

happy to undo it but typically i just stick to named everywhere in my projects

no need! I'm convinced

@lilasquared
Copy link
Author

I wonder where we can find these!!

I can always snap a pic or share them with you individually. I know they are in the tabletop sim version of this that I use so maybe I can snag them from there somehow 😅

@lilasquared
Copy link
Author

oh boy i am blind, the Extra! Extra! rules are in the PDF I already shared. Yikes 🤦🏼‍♂️

@sorahn
Copy link

sorahn commented Mar 9, 2022

Hello @ymichael, I took a stab at fixing the header title length with a Textfit component. It will find the header titles that are too long and shrink them down until they fit. In doing so I re-wrote the card header a bit to use flexbox instead of absolute positioned icons.

What do you think?

Edit: I probably should attach pictures when asking someone to take a look...

All Legends:
Screen_Shot_2022-03-09_at_03 13 21

McGregor's Market (down to 11px font):
Screen Shot 2022-03-09 at 12 23 08

Oleander's Opera House: (down to 9px)
Screen Shot 2022-03-09 at 12 24 10

@sorahn
Copy link

sorahn commented Mar 9, 2022

@elynnlee You mentioned working on the styles Later, I'm a UI/UX developer by trade and I was just poking around a bit to see what I could do. I'm not sure what you guys had in mind as the project progresses, but I added a bit of css grid to try and spruce the joint up a bit. Here's an idea I was working with just to see how it turned out.

Still very much WIP, but the general idea would be to build a game board that is roughly the same set of areas as the game itself. What do you think?

Screen Shot 2022-03-09 at 17 01 10

BTW, I noticed that the game test page only has 3 forest locations instead of 4, but what I was going to do was split them to either side of the meadow, and move the basic locations above the board too. Maybe we could split the locations, basic on one side (smaller and doubled up since there are 6), and forest locations on the other? Just trying things out.

@ymichael
Copy link
Owner

Hello @ymichael, I took a stab at fixing the header title length with a Textfit component. It will find the header titles that are too long and shrink them down until they fit. In doing so I re-wrote the card header a bit to use flexbox instead of absolute positioned icons.

Still very much WIP, but the general idea would be to build a game board that is roughly the same set of areas as the game itself. What do you think?

hey @sorahn, can we move this and the board layout changes into a separate PR that doesn't depend this PR since they are mostly unrelated? It would be much easier to review, give feedback and land the changes that way. Thanks!

@sorahn
Copy link

sorahn commented Mar 19, 2022

Hello @ymichael, I took a stab at fixing the header title length with a Textfit component. It will find the header titles that are too long and shrink them down until they fit. In doing so I re-wrote the card header a bit to use flexbox instead of absolute positioned icons.

Still very much WIP, but the general idea would be to build a game board that is roughly the same set of areas as the game itself. What do you think?

hey @sorahn, can we move this and the board layout changes into a separate PR that doesn't depend this PR since they are mostly unrelated? It would be much easier to review, give feedback and land the changes that way. Thanks!

Oh of course, I'm not making the changes to the layout in his actual pull request. The only change I made was to add the text-fit component to shrink down the names of the longer card titles. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants