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

The (Editor and Collection) Merge #108

Merged
merged 52 commits into from
Mar 1, 2024
Merged

Conversation

eviterin
Copy link
Collaborator

@eviterin eviterin commented Jan 20, 2024

Context (Problem, Motivation, Solution)

Editor was added in my previous PR. This editor was based on the Collection view, and shared a lot of component and functionality. Instead of maintaining the two, I now combined them.

Merge Collection and Editor

Describe Your Changes

Both Collection and Editor had these two components:
On the left: Search/Filter panel
Middle: Card collection

In Collection:
On the right: Player's completed decks

In Editor:
On the right: Player's deck that is under construction

Each of these components have been factored out into their own component.
Editor has been removed, and the Collection now keeps track on whether the player is in editing mode or not.
Index file has been updated to remove the editor shortcut.

Additionally, variables within the collection view have been reordered and grouped by the functionality they support.

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
  • I wrote tests for my new features, or added regression tests for the bug I fixed

Testing

Exploration testing to make sure there are is no regression within the Collection view.

@eviterin eviterin changed the title The merge The (Editor and Collection) Merge Jan 20, 2024
@norswap
Copy link
Member

norswap commented Jan 22, 2024

Is this ready for review? Just tried it and seems to have quite a few problems (e.g. deck doesn't appear in list when you add a new deck) + unused functions in collections.tsx.

Tip: you can mark the PR as "draft" if it's not ready for review (that way other people can have a look if they want to, but they know it's not blocking!)

@norswap norswap marked this pull request as draft January 22, 2024 01:08
@eviterin
Copy link
Collaborator Author

I ran eslint and resolved all warnings. Sorry for that will try to do better in the future..

@norswap I tried cloning my PR fresh and still cannot reproduce the issue you're having with decks not appearing in the list.
The following steps I take:

  1. Navigate to http://localhost:3000/?index=1
  2. Select Collection
  3. Select New Deck
  4. Add cards and a name and press Save
  5. Assert that the deck I created is on the deck list.

Keeping this issue in draft until I know more.

@norswap
Copy link
Member

norswap commented Jan 22, 2024

Might be on my side, maybe I need to rerun make setup, will report back!

@norswap
Copy link
Member

norswap commented Jan 22, 2024

I can reprod, both on Chrome & Firefox, even if I clone fresh.

To be clear, after creating the deck, I expect it to show in the right sidebar, instead, only "New Deck" is shown.

Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Posting some comments I'd enqueued — not a complete review

@eviterin eviterin marked this pull request as ready for review January 24, 2024 16:55
Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Will continue reviewing tomorrow, here's a first batch of comments.

Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

Nice work! Let's get the comments sorted and we can merge this :)

@eviterin
Copy link
Collaborator Author

eviterin commented Jan 27, 2024

Thanks for the review @norswap.
I have now added commits for each actionable comment (reacted with a 👍)

Edit: not ready for re-review. Make check is killing me

@eviterin eviterin marked this pull request as draft January 28, 2024 18:07
@eviterin eviterin changed the base branch from master to ns/ui-fixes January 28, 2024 19:10
@eviterin eviterin changed the base branch from ns/ui-fixes to master January 28, 2024 19:11
@eviterin eviterin marked this pull request as ready for review January 28, 2024 19:11
@eviterin eviterin marked this pull request as draft February 7, 2024 15:57
@eviterin
Copy link
Collaborator Author

eviterin commented Feb 8, 2024

Minor CSS issues now with Aritra's changes, but will be fixed in upcoming PR for onchaining decks.

@eviterin eviterin marked this pull request as ready for review February 8, 2024 13:41
Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

This is progressing nicely! We're dealing with the finer details now.

Also don't forget to rebase regularly!

packages/webapp/src/components/collection/deckPanel.tsx Outdated Show resolved Hide resolved
packages/webapp/src/components/collection/deckPanel.tsx Outdated Show resolved Hide resolved
packages/webapp/src/components/collection/deckList.tsx Outdated Show resolved Hide resolved
packages/webapp/src/components/collection/deckPanel.tsx Outdated Show resolved Hide resolved
packages/webapp/src/components/collection/deckPanel.tsx Outdated Show resolved Hide resolved
Copy link
Member

@norswap norswap left a comment

Choose a reason for hiding this comment

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

The history is a bit of a mess, but I think everything from the first "make check issues" to the end can be squashed together via an interactive rebase. Let's sync on telegram, maybe do a call and I'll walk you through how to do that.

Regarding the UI, we're progressing but not quite there yet. The textfield still overflows and the buttons don't flow vertically when needed. I suggest playing with resizing the window to see how it behaves!

packages/webapp/.vscode/settings.json Outdated Show resolved Hide resolved
@norswap
Copy link
Member

norswap commented Mar 1, 2024

Global feedback:

  • behaviour much better now!
  • kinda looks ugly
    • not enough vertical spacing between text field & button
    • would be good to set a default deck name so we don't start with the red border
      • like Deck #X where X is the number of existing decks + 1
    • buttons should probably look more like the starting page button
      • in particular, reduce border size
      • would be good to have a different color for accept and cancel? (not sure)
    • add space between tick mark / cross mark and text
    • add vertical spacing between "New Deck" and listed decks
      • same, reduce border width
      • make "New Deck" and listed deck more visually distinct
        • keep "New Deck" as button but make listed deck more visually similar to the cards in the deck card list
  • bugs
    • when editing an existing deck, the border shows as red until edited (but deck can still be edited & saved correctly)
  • enhancements
    - grey out "save" if nothing in the deck changed, or if something is invalid (rn only invalid deck name)
    - the two panels (deck list & card lists) are inconsistent: deck list shows a scrollbar when it gets too small, whereas the card list doesn't
    - we should probably do the scrollbar for both (but keep the buttons reflowing when reducing size though!)
    - NOTE: chrome doesn't actually display the scrollbar but the content is scrollable horizontally in decklist, not in the card list still
    - would be good to show the scroll bar on all browsers, but this is the least important item on this list

Still, this is pretty usable, so I'm going to merge this now, so we can enable prettier globally. Then I think the next priorities should be:

  • implement contract interop
  • redo the layout according to Overhaul card collection display #93 (we don't need the filters atm and it looks jank) — tackle the above items at the same item if still relevant in the new layout

@norswap norswap merged commit 2fb40c1 into 0xFableOrg:master Mar 1, 2024
1 check passed
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.

2 participants