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

[UI] Public adventures view #4757

Merged
merged 18 commits into from
Dec 12, 2023
Merged

[UI] Public adventures view #4757

merged 18 commits into from
Dec 12, 2023

Conversation

hasan-sh
Copy link
Collaborator

@hasan-sh hasan-sh commented Nov 15, 2023

Fixes #4604

Info

  • Public adventures are listed as cards
  • Each card contains basic information about the adventure.
    • creator, level, clones (# of times it's created), last update date, and tags
  • These are sorted by currenUser's adventures

Features and Todo

  • Teachers can navigate to their adventures by clicking on the title or edit button.
  • Teachers can clone or view adventures of others
    • if they clone, a new copy of that adventure is created
    • the user gets a notification that cloning succeeded
    • the update is reflected by showing the newly cloned adventure only thus
    • the other teacher's adventure that's cloned now, won't be visible.
  • Teachers can use 3 filters:
    • Search
    • Language
    • Tags
    • Country?
    • User?
    • Level?
  • add tests
  • allow filtering through url

- separate ts functions
- more options; hide empty filters, links, info
- more info to db; more data=power
@ghost
Copy link

ghost commented Nov 15, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@Felienne
Copy link
Member

Felienne commented Nov 16, 2023

I love the tile UI @hasan-sh, looks very good!

image

@Felienne
Copy link
Member

A few random thoughts:

Should we add "public" as a tickmark to the adventure overview:

image

(and maybe also show which ones are cloned?)

@Felienne
Copy link
Member

If I click clone, the tile changes, which I found a bit confusing:

image

What does the bin mean, will it delete the adventure or the clone? The error message does not help me grasp the meaning
image

I think we can do without the bin button here? If people want to remove the adventure, they can do so from their adventure overview?

The same might be true for the add tags link, for which it is not clear if we are adding the tags to the original adventure or the close.

Also it is a bit confusing that it leads to the same edit view as the title link. Maybe we can make one link at the bottom with something like "edit directly"?

@hasan-sh
Copy link
Collaborator Author

A few random thoughts:

Should we add "public" as a tickmark to the adventure overview:

image (and maybe also show which ones are cloned?)

Thought of that and put them in that table, but it got too crowded...

@hasan-sh
Copy link
Collaborator Author

A few random thoughts:
Should we add "public" as a tickmark to the adventure overview:
image
(and maybe also show which ones are cloned?)

Thought of that and put them in that table, but it got too crowded...

but the tables now have cloned_from and cloned_times (i.e. how many times is an adventure cloned!) for future ideas/usage.

@Felienne
Copy link
Member

Thought of that and put them in that table, but it got too crowded...

Fair enough! We will think about a redesign of this page for the future and keep it in mind!

but the tables now have cloned_from and cloned_times (i.e. how many times is an adventure cloned!) for future ideas/usage.

Oooowwww, good thinking!! We could add cloned_times to the tile view so teachers can see popular adventures?

@hasan-sh
Copy link
Collaborator Author

hasan-sh commented Nov 16, 2023

If I click clone, the tile changes, which I found a bit confusing:

image What does the bin mean, will it delete the adventure or the clone? The error message does not help me grasp the meaning image

I think we can do without the bin button here? If people want to remove the adventure, they can do so from their adventure overview?

When a user clones an adventure, we create a brand new one for that user. I did this because we weren't sure whether cloned adventures should be syncronized. I think they shouldn't. So, the card changes because the adventure now belongs to the current user!

The same might be true for the add tags link, for which it is not clear if we are adding the tags to the original adventure or the close.
the main reason why i added this is to entice users to add tags since it's a new feature and great for data analysis later on!!

Also it is a bit confusing that it leads to the same edit view as the title link. Maybe we can make one link at the bottom with something like "edit directly"?

This was really temporary! I could add a button instead of the delete btn, but perhaps we want to change and add more things to this public adventures view; e.g., add adventures to classes for one or multiple levels.

@hasan-sh
Copy link
Collaborator Author

hasan-sh commented Nov 16, 2023

Fair enough! We will think about a redesign of this page for the future and keep it in mind!

INDEED! For instance, a card that has some info icon when clicked, it would show more info (in a modal) and/or direct to different places; same for classes!

@hasan-sh
Copy link
Collaborator Author

Oooowwww, good thinking!! We could add cloned_times to the tile view so teachers can see popular adventures?

Yess sure! Shall i add it as a filter? or just sort adventures by how many times they're cloned in the backend?

@Felienne
Copy link
Member

Oooowwww, good thinking!! We could add cloned_times to the tile view so teachers can see popular adventures?

Yess sure! Shall i add it as a filter? or just sort adventures by how many times they're cloned in the backend?

Let's for now just put it as info on the tiles? We can do more if we have thought about it a bit more :)

@Felienne
Copy link
Member

When a user clones an adventure, we create a brand new one for that user. I did this because we weren't sure whether cloned adventures should be syncronized. I think they shouldn't.

I agree!

So, the card changes because the adventure now belongs to the current user!

Ah I see. I understand the model now but I am not sure that makes sense for a user. Maybe we should have a modal that says: adventure cloned, do you want to go to edit? And then yes brings you to the edit page and no brings you back to search (closes the modal).

This was really temporary! I could add a button instead of the delete btn, but perhaps we want to change and add more things to this public adventures view; e.g., add adventures to classes for one or multiple levels.

Ok, never mind my comments about the bin then. Indeed, there could be a button with more options (later)

templates/public-adventures.html Show resolved Hide resolved
templates/public-adventures.html Show resolved Hide resolved
templates/htmx-adventure-card.html Show resolved Hide resolved
@@ -585,6 +585,9 @@ def batch_get_adventures(self, adventure_ids):
keys = {id: {"id": id} for id in adventure_ids}
return ADVENTURES.batch_get(keys) if keys else {}

def get_public_adventures(self):
Copy link
Member

Choose a reason for hiding this comment

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

Let's not forget to add this table to prod and alpha when this gets deployed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the adventures table which exists on prod, right? Or does that have to be changed when we change its schema? Because I did include a new index to the table!

@jpelay
Copy link
Member

jpelay commented Nov 16, 2023

Hi! Just left a few comments regarding the code :D I like the way this is looking so far. Great idea!

@Felienne Felienne changed the title [UI idea] Public adventures view [UI] Public adventures view Nov 17, 2023
@jpelay
Copy link
Member

jpelay commented Dec 4, 2023

Hello, @hasan-sh!

As we agreed upon during the meeting, for this PR to move forward, we first need to change the existing design of the Explore Programs' cards. Also we need a button here to flag submissions to the admin, for moderation purposes.

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

We decided to merge this to main and use the styles for replacing the old styles!

@jpelay
Copy link
Member

jpelay commented Dec 4, 2023

@hasan-sh Can you solve the conflicts 🙏 ?

Copy link
Contributor

mergify bot commented Dec 4, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@jpelay
Copy link
Member

jpelay commented Dec 8, 2023

@hasan-sh Do you know why so many Cypress tests are failing in this PR? A bunch of stuff that doesn't even has to do with your changes, so maybe there was an error when merging?

@Felienne
Copy link
Member

Felienne commented Dec 9, 2023

Yeah hopefully the new Cypress Cloud access will help us to figure out what is going on!

Copy link
Contributor

mergify bot commented Dec 12, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit c831922 into main Dec 12, 2023
11 checks passed
@mergify mergify bot deleted the pub-adventures branch December 12, 2023 19:31
Copy link
Contributor

mergify bot commented Dec 12, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

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.

[UI idea] Add search adventure by tag
3 participants