Skip to content
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

Issue 7493: Add credit cost of cards alongside their names and influence in deck builder #7829

Merged
merged 11 commits into from
Oct 29, 2024

Conversation

heyshiloh
Copy link
Contributor

@heyshiloh heyshiloh commented Oct 18, 2024

Hello,

This PR contains an initial implementation of #7493

Closes #7493

Screenshot 2024-10-18 at 11 08 53 PM

This PR adds the credit/mu cost of applicable cards to the Deck Builder decklist panel

Description of change:

  • adds toggle options to session app-state in decklist module
  • Conditionally adds wrapped and formatted credit/mu cost content to line in decklist
  • dynamically adjusts css using edit state variable
  • add hover background for further readability improvements

Methodology:

  • Using the existing display pattern for cost icons will allow for users to quickly understand what this additional information is
  • toggle selections will default to false on page load but persist in-session
  • uses absolute position to avoid vertical adjustment to line-height and other properties
  • per discussion, cost content is right-aligned

Considerations and potential next steps:

  • Hover highligting opens up the potential to replace fake-link functionality to a larger area for hovering
  • We could pin the card preview/expand to the top right of the screen as it is in the gameboard
  • alongside this, the text items that are displayed in Cards could be a cool toggle for a fully integrated deckbuilding experience in one tab

@heyshiloh heyshiloh changed the title Issue 7493: Add rez/play cost of cards alongside their names and influence Issue 7493: Add rez/play cost of cards alongside their names and influence in deck list Oct 18, 2024
@heyshiloh heyshiloh changed the title Issue 7493: Add rez/play cost of cards alongside their names and influence in deck list Issue 7493: Add rez/play cost of cards alongside their names and influence in deck builder Oct 18, 2024
@heyshiloh heyshiloh changed the title Issue 7493: Add rez/play cost of cards alongside their names and influence in deck builder Issue 7493: Add credit cost of cards alongside their names and influence in deck builder Oct 18, 2024
@NoahTheDuke
Copy link
Collaborator

looks great. This should be toggleable from within the deckbuilder, defaulting to off.

@heyshiloh
Copy link
Contributor Author

@NoahTheDuke Sounds good, thoughts on any other costs such as memory cost or advancement requirement that would be useful to see inline?

@NoahTheDuke
Copy link
Collaborator

I don't know how you'd include that without making it look incredibly messy. I already have reservations about showing credit cost which is why I suggested defaulting to off.

You're welcome to try it out tho, we can see the results and discuss.

@heyshiloh
Copy link
Contributor Author

@NoahTheDuke make sense to me, I just thought if we were already making toggle-able views its worth considering other relevant information

for the sake of conversation, here is what it might look like

Screenshot 2024-10-18 at 1 31 48 PM

@NBKelly
Copy link
Collaborator

NBKelly commented Oct 18, 2024

That's looking pretty neat. I think having a toggle for each might be nice. I also wonder if we might be better served with the metadata being right-aligned (like a table) for readability (influence is probably fine where it is, since it's only active on 2-3 cards usually, and the number is the unit - but costs/mu are active on all of them, and include a unit too, so it looks less tidy)

@heyshiloh
Copy link
Contributor Author

heyshiloh commented Oct 18, 2024

@NBKelly by right-aligned do you mean to the right of influence or fully alighted to the right side of the container? I think that would be harder to read. Since cost would be present on cards, if we moved the costs to the right of influence on toggle it would end up like

name cost
name influence cost

whereas in the current state of this feature it would be

name cost
name cost influence

I think the latter would be easier to read. Thoughts?

@NBKelly
Copy link
Collaborator

NBKelly commented Oct 18, 2024

Something like

3 Anansi •••••••••              8cr
1 Ice Wall                      1cr
1 Trebuchet                     7cr
----------------------------------------
3 Corroder ••••••           1mu 2cr
1 Ika                       1mu 0cr
1 Stargate ••••             2mu 4cr

That's entirely my preference though, you don't need to do that if you don't like it.

@heyshiloh
Copy link
Contributor Author

@NBKelly i'll draft up both and ask some friends to see if they have any preference. I sometimes find myself highlighting lines on webpages when there is text floating out in space. for now i'll work on the toggle and let you know what it looks like!

@heyshiloh
Copy link
Contributor Author

Corp non-edit view
Screenshot 2024-10-18 at 10 42 40 PM
Screenshot 2024-10-18 at 10 42 59 PM

Corp edit view
Screenshot 2024-10-18 at 10 43 57 PM
Screenshot 2024-10-18 at 10 44 15 PM

Runner non-edit view
Screenshot 2024-10-18 at 10 45 27 PM
Screenshot 2024-10-18 at 10 45 47 PM
Screenshot 2024-10-18 at 10 47 41 PM

Runner edit view
Screenshot 2024-10-18 at 10 48 47 PM

For fun, to visualize alignment in the div i turned on a hover-background for .line and i actualy like it for visibility purposes in moving my pointer over the .fake-link and to see .card-cost association. Can remove this, make it another toggle option, or leave it in there 😁

Screenshot 2024-10-18 at 10 43 24 PM

@heyshiloh
Copy link
Contributor Author

The cool thing about it is that with the css config i have here, it does not affect vertical distance at all when toggling it on and off, so it just appears/disappears inline and doesnt break up the visual

@NBKelly
Copy link
Collaborator

NBKelly commented Oct 19, 2024

That's looking pretty damn neat, good work. The "highlight" on mouseover is actually pretty sick too, I say keep it if you like it.

@heyshiloh
Copy link
Contributor Author

another thing that came up while working on this - if the hover background seems like a good add, maybe hovering over the the div could select that card and pin the card preview on the top right corner of the screen like on the gameboard.

I wrote this down in my list of feature ideas at least

@heyshiloh
Copy link
Contributor Author

@NBKelly thank you!! glad you like it. Going to double check the code here to make sure I dont have print statments etc, but both of my open PRs are review ready as far as logic goes.

@heyshiloh
Copy link
Contributor Author

Screenshot 2024-10-18 at 11 07 09 PM
make css prettier

@heyshiloh
Copy link
Contributor Author

cleaned code updated PR description ready for review

@NoahTheDuke
Copy link
Collaborator

tbh this is great, much better than i thought it would be. thanks for the effort

@heyshiloh
Copy link
Contributor Author

Thanks 😁 Glad to help out

src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
src/cljs/nr/deckbuilder.cljs Outdated Show resolved Hide resolved
@NoahTheDuke NoahTheDuke merged commit 863467f into mtgred:master Oct 29, 2024
3 checks passed
@heyshiloh heyshiloh deleted the nr-7493 branch October 29, 2024 18:26
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.

Proposal: add rez/play cost of cards alongside their names and influence
3 participants