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

Hide default deck when empty #3492

Closed
wants to merge 14 commits into from

Conversation

taylorobyen
Copy link
Contributor

@taylorobyen taylorobyen commented Oct 12, 2024

Closes #3334

Changes

  • Default deck hidden when empty and instead displays "No Decks"
    image
  • Deck preset counts ignore default deck if empty

I have yet to implement the proper translation for the "No Decks," popup, wanted to first get a pulse check if this was still the direction we wanted to go with this.

I have a few additional items I would like to add that weren't specifically mentioned:

  • When add menu clicked notify user to create a deck and open up the deck creation window instead
  • When deck is created when currently 0 decks automatically select it as the current deck (more intuitive when opening browser than having a blank ghost deck selected)

@taylorobyen taylorobyen marked this pull request as ready for review October 12, 2024 14:14
@taylorobyen taylorobyen marked this pull request as draft October 12, 2024 14:14
@brishtibheja
Copy link
Contributor

Just voicing my opinion on this, I think it would be nicer to have a CTA like "Add a Deck" instead of "No Decks". There are the buttons on bottom saying the same, but IMO "No Decks" is a bit uninformative. Still looks great!

rslib/src/deckconfig/update.rs Outdated Show resolved Hide resolved
rslib/src/decks/tree.rs Outdated Show resolved Hide resolved
@dae
Copy link
Member

dae commented Oct 15, 2024

The deck list is shared by all clients, so any CTA needs to be worded in a client-agnostic way, like "Add a deck to get started"

@taylorobyen
Copy link
Contributor Author

CTA is added:
image

@taylorobyen
Copy link
Contributor Author

taylorobyen commented Oct 16, 2024

When attempting to add cards, this popup appears to notify the user to create their first deck:
image

It then opens the create deck dialogue for them:
image

The add dialogue is then opened with the newly created deck selected:
image

If the user cancels out of the deck creation without creating a deck the add dialogue will not open.

@taylorobyen taylorobyen marked this pull request as ready for review October 16, 2024 21:36
@dae
Copy link
Member

dae commented Oct 21, 2024

This involved a few more UI tweaks than I was hoping for at the moment. There's plenty of room for improvement here still, but I don't think we should be tackling much on the UI end until we can get migrated over to shared Svelte code.

In the mean time, some suggestions for simplifying this a bit:

Automatically opening the add deck dialog doesn't teach the user how to add a deck so they know how to add a second one in the future. It also doesn't teach them what a deck is, or what a good name would be. And the CTA pushes the user towards adding an empty deck, when the majority of users will be starting out with a shared deck instead.

Instead, I think we should:

  1. change the CTA to 'Add a shared or empty deck to get started'
  2. make it text, not a button, so the user goes looking for the buttons below
  3. if the user clicks on 'add' before they've added any decks, instead of a pop-up, automatically add a 'my first deck' (a normal one, not id=1). I suggest is_deck_available() by moved into the Rust layer, and converted to ensure_deck_available(). It can then take care of the check and update in one go, and the three clients can then invoke it as they open the add screen.

WDYT?

@@ -242,6 +227,26 @@ impl From<DeckTreeNode> for LegacyDueCounts {
}
}

fn hide_default_deck(node: &mut DeckTreeNode) {
let mut idx = 0;
while idx < node.children.len() {
Copy link
Member

Choose a reason for hiding this comment

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

Iterating over list indices is not very idiomatic in Rust. You should just loop over &mut node.children instead

@taylorobyen
Copy link
Contributor Author

Automatically opening the add deck dialog doesn't teach the user how to add a deck so they know how to add a second one in the future

instead of a pop-up, automatically add a 'my first deck'

These statements conflict on the strategy we want to take. As a user I would find it more convenient to be given the chance to name the deck what I want, but we could also simply tell them to create/import a deck first.

@dae
Copy link
Member

dae commented Oct 21, 2024

My suggested CTA is intended to nudge the users towards the buttons at the bottom. My thinking was that if user instead clicks Add on top thinking that's what they need to do, they probably don't want to be thinking about a deck name. But instead of automatically adding one, simply repeating the CTA in a pop-up so the user must find the buttons could work too.

@dae
Copy link
Member

dae commented Nov 5, 2024

What do you think?

@taylorobyen
Copy link
Contributor Author

Will open this in the future if I find more time.

@taylorobyen taylorobyen closed this Nov 5, 2024
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.

Hide default deck by default
3 participants