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

Add more parametrisation to the box partial #1451

Open
swombat opened this issue May 10, 2024 · 2 comments
Open

Add more parametrisation to the box partial #1451

swombat opened this issue May 10, 2024 · 2 comments

Comments

@swombat
Copy link

swombat commented May 10, 2024

A few features/adjustments would be nice, and can be done in one fix, if this is worth doing (I've already done these locally):

1. don't display title/description section if both are missing

Currently, the box starts with the following code:

  <div class="py-6 px-8 <%= title_padding %> space-y-2 <%= 'border-b shadow-sm dark:border-slate-600' if divider %>">
    <% if partial.title? %>
      <h2 class="text-xl font-semibold dark:text-white <%= title_size %>">
        <%= partial.title %>
      </h2>
    <% end %>

    <% if partial.description? %>
      <p class="text-slate-400 font-light leading-normal">
        <%= partial.description %>
      </p>
    <% end %>
  </div>

So if neither title nor description are provided, you get a massive blank space at the top. Kinda fugly, so I fixed it like this:

  <% if partial.title? || partial.description? %>
    <div class="py-6 <%= title_padding %> space-y-2 <%= 'border-b shadow-sm dark:border-slate-600' if divider %>">

      <% if partial.title? %>
        <h2 class="text-xl font-semibold dark:text-white <%= title_size %>">
          <%= partial.title %>
        </h2>
      <% end %>

      <% if partial.description? %>
        <p class="text-slate-400 font-light leading-normal">
          <%= partial.description %>
        </p>
      <% end %>
    </div>
  <% end %>

2. Shortcut buttons:

Also in the header, having shortcut buttons at the top right of the card seems like a useful reusable pattern, like a show/hide invisible, or other utility buttons:
image

This is easily done by further updating the following to the box heading section:

  <% if partial.title? || partial.description? || parial.shortcut_buttons? %>
    <div class="py-6 <%= title_padding %> space-y-2 <%= 'border-b shadow-sm dark:border-slate-600' if divider %>">
      <% if partial.shortcut_buttons? %>
        <div class="float-right">
          <%= partial.shortcut_buttons %>
        </div>
      <% end %>

      <% if partial.title? %>
        <h2 class="text-xl font-semibold dark:text-white <%= title_size %>">
          <%= partial.title %>
        </h2>
      <% end %>

      <% if partial.description? %>
        <p class="text-slate-400 font-light leading-normal">
          <%= partial.description %>
        </p>
      <% end %>
    </div>
  <% end %>

3. Make pagy position parametrisable

If the list is longer than the screen (e.g. the default 20 items), then when you're looking at the top of the page you won't know that there are new items in the list (e.g. if they're being added by the backend).

Luckily pagy has no issues with being displayed twice on the page. I solved it by having 2 pagys. One above the body:

  <% if pagy && pagy.pages > 1 %>
    <div class="pb-7 space-y-7">
      <div class="sm:flex">
        <% if pagy && pagy.pages > 1 %>
          <div class="flex-0 mt-3 sm:mt-0">
            <%== pagy_nav(pagy) %>
          </div>
        <% end %>
      </div>
    </div>
  <% end %>

And one below:

  <% if partial.actions? || (pagy && pagy.pages > 1 && pagy.in > 10) %>
    <div class="pb-7 space-y-7">
      <div class="sm:flex">
        <div class="flex-1 space-x">
          <%= partial.actions %>
        </div>
        <% if pagy && pagy.pages > 1 %>
          <div class="flex-0 mt-3 sm:mt-0">
            <%== pagy_nav(pagy) %>
          </div>
        <% end %>
      </div>
    </div>
  <% end %>

To avoid clutter, the bottom one only shows if there are more than 10 items in the list.

For BulletTrain I would make this parametrisable. Add a parameter to the box partial of :pagy_placement, defaulting to :bottom as that is the current default. But also offer the option of :top, :both or :none to disable pagy entirely.

@jagthedrummer
Copy link
Contributor

jagthedrummer commented May 14, 2024

@swombat I think this be a good candidate for 3 smaller PRs instead of one big one.

Item 1 seems like a solid fix for a bug.

Item 2 seems like an interesting addition, but I'd like a chance to play with it a bit (in a branch/PR) just to make sure that it will behave under a variety of circumstances.

Item 3 I'm not so sure about. Generally I'm not a fan of top-of-list pagination controls, and I think users are used to looking for them at the bottom of the list. I also don't think it's great for usability if the bottom pagination controls are sometimes there and sometimes not depending on the size of the list you're looking at. I'd be happy to try it out in a PR, but I can't promise that we'd merge that one.

@swombat
Copy link
Author

swombat commented May 18, 2024

Cool. I'll give those a go then.

On item 3, I see what you're saying. The problem I'm trying to solve is I literally watched a user recorded session where they couldn't figure out there were new results appearing (created by an AI) because the pagy was at the bottom, off-screen, so the only visual change as new results were added was... nothing. So they just sat there waiting for something to happen and nothing happened.

The reason to then make the bottom one only appear with more than 10 results was that if there's very few results and two paginators it looks quite clunky.

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

No branches or pull requests

2 participants