Skip to content

Pad date range sliders to fix search facet animation issues#1574

Merged
tdonohue merged 1 commit intoDSpace:mainfrom
ybnd:Fix-search-filter-date-range-slider-collapse-animation
Apr 1, 2022
Merged

Pad date range sliders to fix search facet animation issues#1574
tdonohue merged 1 commit intoDSpace:mainfrom
ybnd:Fix-search-filter-date-range-slider-collapse-animation

Conversation

@ybnd
Copy link
Copy Markdown
Member

@ybnd ybnd commented Mar 22, 2022

References

Add references/links to any related issues or PRs. These may include:

Description

Currently, date range sliders in search facets cover the full width of the facet, which causes the handles to jut out a little bit to the left/right.
This works fine, but causes those handles to get pushed behind the facet's padding during the collapse/expand animation.

This PR fixes this by padding the sliders so that the ends of handles at the min/max positions are flush with the slider itself

date-range-slider-animation-after.mov

Instructions for Reviewers

Confirm that

  • the slider handles are no longer cut in half during the animation when positioned at the ends of the slider
  • the date range sliders still work as they did

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@ybnd ybnd self-assigned this Mar 22, 2022
@ybnd ybnd added component: Discovery related to discovery search or browse system quick win Pull request is small in size & should be easy to review and/or merge labels Mar 22, 2022
@ybnd ybnd marked this pull request as ready for review March 22, 2022 18:48
@ybnd ybnd changed the title Pad date range sliders Pad date range sliders to fix search facet animation issues Mar 22, 2022
@ybnd
Copy link
Copy Markdown
Member Author

ybnd commented Mar 22, 2022

One possible caveat: the current look is kinda nice too

20220322-195207

But IMO the animation fix is worth it.

@tdonohue tdonohue added this to the 7.3 milestone Mar 22, 2022
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label Mar 22, 2022
@tdonohue tdonohue self-requested a review March 24, 2022 16:00
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@ybnd : Overall, I agree that the slight alignment issue on main looks a little odd. However, I found that with this tiny PR, the right slider cannot be moved all the way to the right side. Here's a screenshot which shows a tiny sliver of green bar on the right of the right slider:

after

If we can make it so the bar doesn't extend past the right slider, then this PR would be fine with me. Otherwise, I feel the current behavior is slightly "better" (even if the range slider is slightly misaligned with the start/end date fields above it).

So handles don't jut out when set to the min/max
@ybnd ybnd force-pushed the Fix-search-filter-date-range-slider-collapse-animation branch from 4d12a29 to dac8524 Compare March 31, 2022 19:47
@ybnd
Copy link
Copy Markdown
Member Author

ybnd commented Mar 31, 2022

Apparently this happened only at some viewport widths, thanks for catching it!

When resizing slowly you can see the position of the element jitter a little, and once in a while the slider juts out from under one of the handles (also on the left side sometimes!).
There's a tradeoff between this and the sides of the handles getting clipped off by the animation, getting it to avoid both issues took a bit of time and pixel peeping.

With the current CSS I haven't been able to replicate either problem.

@tdonohue tdonohue self-requested a review April 1, 2022 20:58
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Thanks @ybnd ! I've tested this PR with your updates, and the new CSS works perfectly for me as well! I tried it in several browsers (Chrome, Firefox, MS Edge) and also various sized windows. It all looks good now.

@tdonohue tdonohue merged commit c1e6976 into DSpace:main Apr 1, 2022
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Apr 1, 2025
DSC-1367

Approved-by: Vincenzo Mecca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge component: Discovery related to discovery search or browse system quick win Pull request is small in size & should be easy to review and/or merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants