mitodl / mit-open

BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

Filled vs Unfilled Bookmarks #1180

Closed ChristopherChudzicki closed 3 months ago

ChristopherChudzicki commented 3 months ago

What are the relevant tickets?

https://github.com/mitodl/hq/issues/4712

Description (What does it do?)

Makes userlist bookmark and learningpath icon filled vs unfilled based on membership in at least 1 list.

Screenshots (if appropriate):

Cards

Screenshot 2024-06-26 at 6 32 13 PM

List Cards

Screenshot 2024-06-26 at 6 32 46 PM

List Cards, Mobile

Screenshot 2024-06-26 at 6 32 57 PM

How can this be tested?

  1. As a logged in user, click the bookmark button on a card to change its membership in "userlists".
    • try this throughout the app, particularly for horizontal "ListCard" (search page) and vertical cards
  2. When the resource belongs to at least one list, the bookmark should be filled.
  3. When the resource belongs to zero lists, the bookmark should be unfilled.
  4. The stafflist button should behave similarly.
  5. Try adding/removing featured courses (homepage "Featured Courses" carousel) to a userlist.
    • in contrast to RC, the carousel resources should maintain their order.
abeglova commented 3 months ago

You should add a little more padding around the list icon, it overlaps the learning format on smaller mobile screens (chrome iphone SE screen shown) this doesn't happen on main

Screenshot 2024-06-28 at 12 28 44 PM
ChristopherChudzicki commented 3 months ago

@abeglova One thing to keep in mind is that normal users only have a single icon—the list icon is only visible to staff.

But even for normal users, this exacerbates the overlap issue more than it should. I'll adjust a bit.

E.g.,

Screenshot 2024-06-28 at 12 57 52 PM
ChristopherChudzicki commented 3 months ago

@abeglova I've tweaked the button positioning to account for button paddings. The cards should look good at 375px with one button, and may look good with 2 buttons depending on date length.

Screenshot 2024-06-28 at 2 37 18 PM Screenshot 2024-06-28 at 2 35 13 PM
ChristopherChudzicki commented 3 months ago

pre-commit.ci run

ChristopherChudzicki commented 3 months ago

Since github says this is mergable, and pre-commit just says

timeout waiting for merge ref

I'm going to merge this.