Deck Editor added #96

eviterin · 2023-12-30T14:21:22Z

Context (Problem, Motivation, Solution)

There needs to be a deck editor in place (see: #40).
Index page updated to redirect to editor. Which has heavily borrowed content from the collections page.

Describe Your Changes

Allow user to select cards from a collection and add it to their deck
Allowsuser to select cards already in the deck to remove them from the deck.
Rename deck
Discard deck
Big card is now updated on hover instead of on select

What's not included?
A place to view the player's decks (I'm thinking collections page)
Logic to save the deck (no onchain transaction)

Checklist

I have performed a self-review of my code
I ran make check and fixed resulting issues
I ran the relevant tests and they all pass
Ran linter and resolved all warnings and errors.
I wrote tests for my new features, or added regression tests for the bug I fixed
Exploratory testing only. Should I be writing tests in e2e using playwright?

Testing

Browse to http://localhost:3000/?index=1
Select Editor
Press save
Assert unable to save with empty name
Add a couple of cards to deck
Rename deck
Press save
Assert no error to save
Press discard
Assert all cards removed

norswap · 2024-01-02T00:57:44Z

Hey Eviterin! Thanks for contributing!

Could you split the diagram and the deck editor into two different PRs to make it easier to review / faster to merge?

Quick feedback on the diagram:

could you distinguish the game creator from the joiner (just setting "Player A initiates createGame" for instance)
the joiner cannot cancel the game at the moment, only the creator
would be good to import the picture directly into the repo instead of linking to a third-party website

I'm semi-off this week, but I'll try taking a look at the deck editor later this week.

eviterin · 2024-01-02T12:24:11Z

Thanks for the feedback.

Against all odds I have been able to split up the pull request in two.
This one is now related to Deck Editor.
And this is related to activity diagram: #97

norswap · 2024-01-03T14:46:13Z

Exploratory testing only. Should I be writing tests in e2e using playwright?

We haven't really been doing this in a while, so it's fine. We probably ought to rewrite playwright tests to not use Metamask in most cases (as that makes it very slow and it's more fragile when updating etc). Future work :')

norswap · 2024-01-03T15:32:05Z

This is pretty nice!

The collection page was also pretty much exploration work, and is even uglier than the rest of the game. We probably won't feature it in the proof of concept. Nevertheless, here's an issue with some ideas of what that facelift could look like.

Adding the deck editor is an interesting additional constraint. I like what you've done, and I think we could integrate it into the proposal above with a right panel (but that just displays when visualizing or editing the deck). One thing I would change is to replace the full-size display of cards in the right panel with a "list" view where only the name of the card (and in time, some logo/colors/decorations for its main characteristics, e.g. creature, color, ...). We could display the full card on hover (either inline or in a popup).

UX Issues

Some UX problems I've seen in testing:

adding more than a few cards to the deck causes a sidebar to appear (good) which causes the size of the cards to change (bad)
renaming a deck causes a new deck to be created, but not the old one to disappear
when editing an existing deck, the deck name is not populated
the clear button is a little bit confusing, it's not quite clear it will remove all cards in the deck
no easy way to get back to the collection view
- I would probably add a button to abort deck creation, which would replace the clear button and navigate back to the collection view
- though as mentioned in the comments above, I think both should be a single page

Card Details

A question I had was how to handle card details display. Right now we can display all details pretty easily but long term that won't be the case.

A hover display works, thought it'd be nice to be able to keep a card displayed and compare with others? Maybe that isn't really needed.

Also where to put that display? In the collection UX overhaul issue, we haven't budgeted space for displaying the card on the left as exists currently. Maybe we could reintroduce that (split the left panel in two halves — maybe allowing to minimize the top half (lists of decks & filters) for small screens.

We could have a floating hover, but that could be janky when trying to move away from it? Maybe it works.

Another option is to require a click for expanded display then have a "Add to Deck / Remove From Deck" button on the expanded display.

Next Steps

I see two ways to build on this:

UX overhaul following the issue & consideration above (and further discussion based on your own ideas!), merging collection/editor
Actually querying the chain for getting and creating the decks. Probably add deck names on chain too! Could also implement the incremental display updates described in the "Data Architecture" section of the issue

This reverts commit 49e85f0, reversing changes made to 271334e.

eviterin · 2024-01-04T09:03:00Z

Hey Norswap. Thank you for the awesome feedback and for sharing your thoughts.
To start, I fully agree on merging collection/editor. There's so much overlap.

There are some things you mention that I don't really have much experience with and might need some pointers on. Marked with (!).

Would you prefer these changes in one PR or can I split it up in three, as proposed below.

#40 related, short term:
Replace cards with their names - keep hover.
Prevent cards in deck to change size when sidebar appears.
Rename deck to remove old deck.
Auto populate the deck name when editing an existing deck.
Remove the clear button.
Shortcut to collection view from editor view.

#40 related short/medium term:
(!) Add deck names on chain
Try out different display options(floating hover, show/hide filters, not having it at all?)

#93 related, medium term:
Merge collection and editor view.
(!) Add filter for inventory cards
(!) List all cards owned

Anything I'm missing?

eviterin · 2024-01-05T00:34:34Z

Fixed the short term stuff.

Although there is a known error: when loading an existing deck, it does not highlight the cards that are in the deck.

Seems that since we are using index to identify cards and we have two views (collection and editor) the ids don't match up. ID 0 in collection is ID 24 in editor, 1 in collection is 25 in editor. And so on - since we have 24 cards.

This issue should resolve itself once editor and collection have been merged.

norswap

Apparently when I submitted my review it did not post the comments I've been making ... sorry about that, at least they're here now!

norswap · 2024-01-03T14:53:35Z

packages/webapp/src/pages/collection.tsx

              {cards.map(card => (
                <div className="m-4 bg-slate-900/50 hover:bg-slate-800 rounded-lg p-4 border-4 border-slate-900"
                     key={`${card.id}`}
                     style={{height: 'fit-content'}}
-                     onClick={() => setSelectedCard(card)}>
+                     onMouseEnter={() => setSelectedCard(card)}>


I'm sure I like this as a UX change.

Edit: I understand it in context, since clicking in the editor adds to / removes from the deck. See discussion in big comment.

I'll try to do some alternative experiments and see what sticks.

norswap · 2024-01-03T14:55:57Z

packages/webapp/src/pages/collection.tsx

I have a feeling the collection and editor pages should probably be unified — make the editor part of the collection page. It'll probably require isolating some components to their own file to avoid a bloated source file though.

Agreed. I'll get on that... at some point.

norswap · 2024-01-03T14:58:47Z

packages/webapp/src/pages/editor.tsx

+import { Address } from "src/chain"
+import { FablePage } from "src/pages/_app"
+import { router } from 'next/router';
+import { useEffect} from 'react';


We're avoiding semicolons in the codebase.

We ought to add a formatter in.

norswap · 2024-01-03T15:03:54Z

packages/webapp/src/pages/editor.tsx

+    if (!isNaN(deckIndex) && decks[deckIndex] != null) {
+      setDeck(decks[deckIndex].cards);
+    }
+  }, [router.query.index, decks]);


I think this might clash with the index parameter used in debug mode to set the wallet address, probably picking another name like deckID for the URL parameter would be better.

norswap · 2024-01-06T17:50:26Z

packages/webapp/src/pages/editor.tsx

-      // Add the new deck to the decks state
-      setDecks(prevDecks => [...prevDecks, newDeck]);
+      // Add as a new deck
+      updatedDecks.push({ name: deckName, cards: deck });


We could avoid mutation here and use sthing like

let updatedDecks: Decks[] = [] ... // first branch decks.toSpliced(originalsDeckIndex,1) ... // second branch decks.toSpliced(originalDeckIndex, 1, { name: deckName, cards: deck })

The big advantage would be avoiding changing the deck ordering.
Anyway, let's not do that, as we'll need to manage the decks on chain anyway :D

norswap · 2024-01-06T17:54:38Z

So I'd say before merging this:

remove semicolons
change "index" URL parameter to something else like "deckID"

The rest of the work can come in the subsequent PR!

Oh also, let's try to avoid merge commits in branches.

Actually right now we rebase everything (even PRs are merged by rebase), but I'm wondering about merging PRs with merge instead of rebase instead.

norswap · 2024-01-06T17:57:29Z

One UX note: the buttons overflow to the right of the textfield, creating a lateral scrollbar — we should avoid that!

eviterin · 2024-01-06T20:18:16Z

Fixed the semicolon stuff and the index renaming!

norswap · 2024-01-07T15:29:17Z

Approved, but needs to be rebased to be mergeable!

eviterin and others added 3 commits December 30, 2023 15:13

Added activity diagram to flow.md

0318460

Deck editor created

79afbd8

Big card now shows card hovered, not selected

9cb871d

eviterin changed the title ~~Added activity diagram to flow.md~~ Deck Editor added Jan 1, 2024

added glow when selecting a card in the editor

28835c5

eviterin and others added 2 commits January 2, 2024 12:55

Removed Flowchart

cae165f

Activity diagram of New Game added

e0a07fc

eviterin added 7 commits January 2, 2024 22:11

Made card details on hover instead of on click

5482243

added panel for decks in the collections view.

c794314

Pressing New Deck in collections now redirects to editor

822b8a9

Moved test decks to app file

2187df8

Save deck implemented

bc3dc49

same card gap in both collection and editor

045ce8b

Enabled possibility to load a deck from collection into editor

271334e

eviterin added 3 commits January 3, 2024 19:46

Indentation fix

5bbbeae

Merge branch 'DiagramUpdate'

49e85f0

Revert "Merge branch 'DiagramUpdate'"

ff2ea69

This reverts commit 49e85f0, reversing changes made to 271334e.

eviterin added 8 commits January 4, 2024 21:47

Removed the clear button

6fe6933

Auto populate the deck name when editing it

77ffa85

Replaced cards in a deck with just the name

3ad3998

Overwrite decks of same name, if already exists

1599a24

Rename deck to remove old deck

4e97c5c

Added cancel button that gets you back to collection

524a567

prevent empty decks from being saved

2d9f8a9

Merge branch 'CollectionOverhaul'

162d929

norswap reviewed Jan 6, 2024

View reviewed changes

eviterin added 2 commits January 6, 2024 21:09

Removed semicolons;

66af583

Changed index to deckID. Added some css to cards in deck display

89f9aaf

norswap approved these changes Jan 7, 2024

View reviewed changes

eviterin added 2 commits January 7, 2024 18:06

Merge branch 'master' into master

0b2d7a1

Merge branch 'master' into master

f13a346

eviterin closed this Jan 10, 2024

norswap mentioned this pull request Jan 14, 2024

Created deck editor #100

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deck Editor added #96

Deck Editor added #96

eviterin commented Dec 30, 2023 •

edited

Loading

norswap commented Jan 2, 2024

eviterin commented Jan 2, 2024

norswap commented Jan 3, 2024

norswap commented Jan 3, 2024

eviterin commented Jan 4, 2024

eviterin commented Jan 5, 2024

norswap left a comment

norswap Jan 3, 2024

eviterin Jan 6, 2024

norswap Jan 3, 2024

eviterin Jan 6, 2024

norswap Jan 3, 2024

norswap Jan 3, 2024

norswap Jan 6, 2024

norswap commented Jan 6, 2024

norswap commented Jan 6, 2024

eviterin commented Jan 6, 2024

norswap commented Jan 7, 2024

Deck Editor added #96

Deck Editor added #96

Conversation

eviterin commented Dec 30, 2023 • edited Loading

Context (Problem, Motivation, Solution)

Describe Your Changes

Checklist

Testing

norswap commented Jan 2, 2024

eviterin commented Jan 2, 2024

norswap commented Jan 3, 2024

norswap commented Jan 3, 2024

UX Issues

Card Details

Next Steps

eviterin commented Jan 4, 2024

eviterin commented Jan 5, 2024

norswap left a comment

Choose a reason for hiding this comment

norswap Jan 3, 2024

Choose a reason for hiding this comment

eviterin Jan 6, 2024

Choose a reason for hiding this comment

norswap Jan 3, 2024

Choose a reason for hiding this comment

eviterin Jan 6, 2024

Choose a reason for hiding this comment

norswap Jan 3, 2024

Choose a reason for hiding this comment

norswap Jan 3, 2024

Choose a reason for hiding this comment

norswap Jan 6, 2024

Choose a reason for hiding this comment

norswap commented Jan 6, 2024

norswap commented Jan 6, 2024

eviterin commented Jan 6, 2024

norswap commented Jan 7, 2024

eviterin commented Dec 30, 2023 •

edited

Loading