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

LWS-138: styling tweaks to filters #1032

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

jesperengstrom
Copy link
Contributor

Description

Tickets involved

LWS-138

Summary of changes

Applied some additional styling to the content in the filter panel.

  • Slightly more highlighted and repositioned totalItems for better readability (?)
  • More visible toggle btn

Still doesn't look at all readable some cases, but if truncation isn't an option, we really should give the panel (back) some more width.

Skärmavbild 2024-05-16 kl  16 26 56

@jesperengstrom jesperengstrom marked this pull request as ready for review May 16, 2024 14:32
@olovy
Copy link
Contributor

olovy commented May 17, 2024

One way to make it more readable could be classic table formatting with alternating background?
(nth-child(even))
Maybe not super pretty but readable.
bild

@johanbissemattsson
Copy link
Contributor

johanbissemattsson commented May 17, 2024

I think it looks nice with the background behind the hits count! Not a fan of the enormous show more/show less buttons. I think it takes too much focus compared to the rest.

Some minor comments:

  • More margin/padding between the facet items are needed. Not much more – just a teeny bit more so they are more distinctly separated.

  • I'm inclided to favour not having any line-breaks in the facets together with text-overflow: ellipsis; – the ellipsis should preferebly be in the middle of the string as the most important separating parts is often in the beginning and at the end. This is sadly not supported natively in CSS so a work-around is needed. So it is probably best done in a later PR. Keeping it on one line would of course require the use of the title attribute so the full text is readable when hovering / readable by screen readers. Smaller text sizes is maybe also needed.

  • The padding to the left of the facet items should also be clickable.

@olovy
Copy link
Contributor

olovy commented May 17, 2024

  • Agree that a teeny bit of margin between items maybe could work for separating them.
  • Could skip the left padding of items and use the whole width? (Perhaps nicer with the chevron to the right then but that is a bigger change). See e.g. finna.fi
  • Agree that show more should be same size or smaller than the items

0.3 rem margin-bottom, no left padding:
bild

finna
Skärmbild från 2024-05-17 10-11-49

@jesperengstrom
Copy link
Contributor Author

jesperengstrom commented May 17, 2024

Ok,

  • Reverted the button
  • Added separating margin between items

Could skip the left padding of items and use the whole width?

I think we then lose the visual hierarchy that is really good for orientation?

I'm inclided to favour not having any line-breaks in the facets together with text-overflow: ellipsis;

Libris Kat has a neat way of ellipsing each part of the name individually. Can't do that now since it's a single string (used for searching), but maybe we can have it both ways later on somehow...

Skärmavbild 2024-05-17 kl  11 53 02.

I still think the filter panel should be allowed 320px width as before (on bigger screens maybe), that would in most cases mean one line break instead of 2, which makes a big difference...

@johanbissemattsson
Copy link
Contributor

johanbissemattsson commented May 17, 2024

Libris Kat has a neat way of ellipsing each part of the name individually. Can't do that now since it's a single string (used for searching), but maybe we can have it both ways later on somehow...

Ah that's also a nice solution!

I still think the filter panel should be allowed 320px width as before (on bigger screens maybe), that would in most cases mean one line break instead of 2, which makes a big difference...

Feel free to make changes to https://github.com/libris/lxlviewer/blob/develop/lxl-web/tailwind.config.js#L117 but it's very much a balacing act as it also affects what works and doesn't works in other places (e.g. the spacing between the resource images and the rest of the contents on the resource page). Alas, I think 320px is too wide for medium sized screens (see attached screenshot).

But I agree that the current state isn't optimal (see second attached screenshot when I zoomed out several steps). We probably need to use clamp

I think it would be good to look at it together next design meeting wednesday!

Skärmavbild 2024-05-17 kl  13 53 04

Skärmavbild 2024-05-17 kl  13 56 37

@johanbissemattsson
Copy link
Contributor

johanbissemattsson commented May 17, 2024

Something like this could maybe work (as clamp doesn't seem to play along with grid columns):

gridTemplateColumns: {
    find: 'minmax(280px, min(400px, 16.67%)) 5fr'
},

@jesperengstrom
Copy link
Contributor Author

Something like this could maybe work (as clamp doesn't seem to play along with grid columns):

I don't experience any big difference, but yeah, let's discuss next week.

Copy link
Contributor

@olovy olovy left a comment

Choose a reason for hiding this comment

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

🚢

@olovy olovy merged commit bec0bc4 into develop May 21, 2024
@olovy olovy deleted the feature/LWS-136-style-filters branch May 21, 2024 10:03
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.

3 participants