internetarchive / openlibrary

One webpage for every book ever published!
https://openlibrary.org
GNU Affero General Public License v3.0
5.26k stars 1.4k forks source link

Unified "Read" Button Dropper #5831

Open cdrini opened 3 years ago

cdrini commented 3 years ago

We have a lot of read-y actions a user can perform; designing these as a dropdown would allow us to expose these options in more places; eg search results, carousels; without taking up a bunch of space.

Options

Borrow / Read, Listen, Search Inside, Download, Locate (meaning check nearby libraries)

Description, Patron Story

Turns the read button into a drop-down menu with more options

saves real estate on the page by grouping similar options (downloads, etc)

all patrons and stakeholders of the books page

Preferably everything is managed using details html element and no js is required, all css.

Relevant Designs

image

Screenshot 2024-05-10 at 8 33 10 AM

https://www.figma.com/file/dYQkIJOJeMo9hx7ewZQH9x/Open-Library%3A-Design-System-%26-Component-Library?type=design&node-id=601-199&mode=design&t=giRw2crBrKKYkWGi-0

Problem Statement

Proposal & Constraints

Actions in dropdown (WIP):

Additional context

Stakeholders

@mekarpeles @cdrini

seabelis commented 2 years ago

Related request from a patron: "Hi! Please add "Want to read" button to search resulta view so that results don't have to opened separately."

jimchamp commented 2 years ago

Decisions made by staff about this:

  1. We want to avoid ever having to show "Not in Library" as a call to action for this button.
  2. We want to include the library links in the redesigned "Read" button. Presently, library links are found in the second item in the sidebar (highlighted below):

library_links

mekarpeles commented 2 years ago

See: https://www.figma.com/file/nVmPYLTaxWWYchVe2ArVbl/Unified-Borrow-Button?node-id=0%3A1

cdrini commented 10 months ago

Here's an iteration on the designs in Mek's figma. We talked about this during our design call and are still uncertain on whether we want it, but if we did, here are some more designs!

image

https://www.figma.com/file/dYQkIJOJeMo9hx7ewZQH9x/Open-Library%3A-Design-System-%26-Component-Library?type=design&node-id=1382-2&mode=design&t=IWiihHe1SPc4LcSm-4

mekarpeles commented 9 months ago

Resolved: We're going to move forward with an implementation based on html details to have a Read button dropdown which at minimum includes:

For version one, let's only do the "Read Dropper" dropdown on the Books Page

In the future, we may want to treat the Read Dropper like the My Books Dropper which uses e.g. async loading when dropper is clicked.

"Preview" option won't be in the menu because we want to standardize how previews/search-inside is done across carousels, search page, and books page -- needs design + discussion, out of scope for this issue.

For the general design, we'll go with @cdrini's variant here: https://github.com/internetarchive/openlibrary/issues/5831#issuecomment-1928913542

mekarpeles commented 7 months ago

Here's a codepen with scaffolding for such a redesign: https://codepen.io/mekarpeles/pen/XWQyjYX

mekarpeles commented 6 months ago

@SivanC how do you feel about tag teaming on this one?

SivanC commented 6 months ago

@mekarpeles Hi, looks promising! :smile: Could you elaborate on the tag team aspect?

mekarpeles commented 6 months ago

@SivanC see if you can implement the read button as a dropper and then @jimchamp and I can help you generalize the button so that it can be added multiple times on a page and have its contents loaded asynchronously while also having basic no-js fallbacks

SivanC commented 6 months ago

Great, I'll start this week and keep you posted on my progress!

SivanC commented 6 months ago

@mekarpeles Hi, I've made a codepen of my own that is a little closer to the Figma design, but there's two big issues I'm still trying to solve:

While researching, I also noticed that there is much discussion around the accessibility of nested interactive elements in summary elements, which I can share if you're interested. Supposedly this style of "details menu" has been popularized by Github but I haven't found much to back that up.

Please let me know what you think! I'll continue to tweak the codepen (especially to try and implement the light blue sub-buttons for e.g. Download) in the coming week and see if I can find answers to the above issues.

mekarpeles commented 6 months ago

@SivanC this is looking fantastic :) really nice work

You could make the cta-content-inner position: absolute which will make its width independent from its parent

It looks re: border radius, you've been able to get the bottom left and right corners rounded correctly :)

SivanC commented 6 months ago

Thanks for the tip, that actually fixes both of the issues! Without position: absolute the corners of the main/dropper button are still unrounded when the dropper is expanded.

mekarpeles commented 6 months ago

The LoanStatus macro handles all of the rending of primary action buttons on the website: https://github.com/internetarchive/openlibrary/blob/master/openlibrary/macros/LoanStatus.html#L43

It makes use of the ReadButton macro, which is the one we want to upgrade: https://github.com/internetarchive/openlibrary/blob/master/openlibrary/macros/ReadButton.html

SivanC commented 5 months ago

EDIT: These issues have been solved with help from rebecca-shoptaw!

@mekarpeles @jimchamp Hi, sorry for my relative radio silence recently, just wanted to give you guys an update on my progress in migrating my codepen design to my local environment.

Here's what it looks like: Closed: image Open: image

Styling issues I'm currently working on:

  1. I tried to read up on specificity and how to override certain style rules in the best way but I'm a little stumped. The two fixes I need to make are to remove the -5px horizontal margin from the summary element (causing the current misalignment) and to set list-style(-type): none for the dropdown content to remove the bullets.

  2. The worst of the issues is the weird layout of the read button with the summary::after pseudo-element. Not sure why it's so wonky when it worked fine in the codepen. I tried putting a variety of display styles on a variety of elements to no avail. You can see that my element inspector shows a strange (to me) occupied space with display: flex enabled for the summary element: With display: flex: image Without flex: image

Other issues I can probably solve by myself but are still WIP are coloring the dropdown arrow and adjusting spacing and hover behavior on the list elements in the dropdown. I'll also begin to work on icons, button functionality and the like when these issues are out of the way. I'll make sure to summarize this again on the call tomorrow. Thanks for taking a look!

rebecca-shoptaw commented 5 months ago

@SivanC Dropping by with some CSS help in case it's still needed!

  1. For list-style troubleshooting, you'll want to make sure you're adding list-style-type: none to whichever element is the ul
  2. I'd recommend adding a flex-grow: 1 to the "Read" part of the button, which makes it automatically take up all the remaining space in the flex container -- if this doesn't work, you can try pairing it with a flex-shrink: 0 and flex-basis: 1 as well! 🙂
SivanC commented 5 months ago

@rebecca-shoptaw Hi thanks for your tips! Flex grow works :D As far as the list styling the issue is that I can see in my element inspector that my styling is being overwritten by other rules; other than that it's being applied correctly. image

rebecca-shoptaw commented 5 months ago

@SivanC Aha! You want to make sure you're applying it to the full list ul element rather than each li element -- i.e. if .cta-dropper__content-list is the class for your ul you want to give the list-style-type: none to that itself, not to its li children.

Edit: Hmm looks like list-style-type should work in both places, if you could include a quick screenshot of what the HTML looks like here, i.e. what the hierarchy is of classes, etc. that would be very helpful!

If that doesn't work, there are some specificity tricks you can try, but fingers crossed that does the trick!

SivanC commented 5 months ago

@rebecca-shoptaw Unfortunately the reason I had it on the li elements was because I thought the same initially and applied it to the ul to no effect. cta-dropper__content-list is the class associated with the ul element and while the styling shows up in the inspector the bullet points remain: image image

Here's the full HTML right now:

<div class="dropdown-button-group">
  <div class="cta-dropper">
    <details class="cta-dropper__details">
      <summary class="cta-dropper__summary">
        <div class="cta-button-group">
          <a href="$(stream_url)" title="$title" class="cta-btn cta-btn--ia cta-btn--available cta-btn--$(action)"
              $:analytics_attr(analytics_action)
              $if loan:
                data-userid="$(loan['userid'])"
              >$label</a>
        </div>
      </summary>
      <div class="cta-dropper__content">
        <ul class="cta-dropper__content-list">
          <li>Listen</li>
          <li>Search Inside</li>
          <li>Check Nearby Libraries</li>
        </ul>
      </div>
    </details>
  </div>
</div>
rebecca-shoptaw commented 5 months ago

@SivanC Thank you!! Ok, it looks like the styling is being inherited because the parent element of all of this has a read-options class, which has its own li rules.

This means you'll want to use selector specificity to overrule the existing li rules in this particular case. It looks like someone else has already encountered a similar issue and fixed it in read-panel.less: https://github.com/internetarchive/openlibrary/blob/9fc1f69e86bbb0ef3fb3595d27b23b0a59d691d2/static/css/components/read-panel.less#L164-L167

So it seems that the simplest course of action would be to add a few lines like this directly above those:

.read-options .cta-dropper__content-list li {
  list-style: none;
}

Effectively you're saying that while all .read-options li elements usually have a list-style: disc, if they are the child of a .cta-dropper__content-list they have a list-style: none.

There may be a more elegant way to do this, but this should do the trick!

You could also try swapping out the .cta-dropper__content-list for a higher up class, i.e. .cta-dropper, as long as that won't cause problems in other foreseeable cta-droppers!

SivanC commented 5 months ago

@rebecca-shoptaw I was about to respond that nothing I tried was working ... until! I noticed that both .read-options and .Tools .btn-notice asked for disc style, so I made a rule that covered both of them according to your suggestion:

.read-options .cta-dropper__content-list li, .Tools .btn-notice .cta-dropper__content-list li {
  list-style: none;
}

image

No more bullet points! :D Also worked to overwrite the negative margins causing the button to be misaligned.

SivanC commented 4 months ago

Update as of 7/26/24 design call, clarifying some questions surrounding dropper content: