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

fix: ensure focused date is visible if overlay is small #809

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

sissbruecker
Copy link
Contributor

@sissbruecker sissbruecker commented Oct 12, 2022

Description

Fixes the overlay content to keep the focused date visible if the month scroller area does not fit a full month. Previously the overlay would only scroll to the top of the month, which means that later days in that month might not be visible. This change adds a new sub-month scroll mode to the overlay that is used when the scroll area does not fit a full month. In that mode the overlay will attempt to keep the week, that the focused date is in, visible.

This is a best effort implementation as the scroller's position is based on months, and the scroll position of a week can not be exactly calculated. I did manual testing on Chrome, FF, Safari and IE, and it seems to work reasonably well for the default Lumo and Material themes.

See video below for a demonstration:

Bildschirmaufnahme.2022-10-12.um.14.23.49.mov

Fixes #vaadin/web-components#4037

Type of change

  • Bugfix

@CLAassistant
Copy link

CLAassistant commented Oct 12, 2022

CLA assistant check
All committers have signed the CLA.

@sissbruecker sissbruecker marked this pull request as draft October 12, 2022 12:59
@sissbruecker sissbruecker marked this pull request as ready for review October 12, 2022 13:48
}, done);
});

it('should scroll when the month is above the visible area', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old test cases for height(visible area) < height(item) are obsolete with the new mode.

Comment on lines +444 to +447
overlay.revealDate(new Date(2021, 1, 15), false);
const positionOf15th = monthScroller.position;
expect(positionOf15th).to.be.greaterThan(initialPosition);
expect(positionOf15th).to.be.lessThan(initialPosition + 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spent some time to create a test that checks if the date element is actually visible in the scroll area, but could not find a reliable setup. The scroller hard-codes the height of an item to 270px, while the test does not load the theme, which leads to an item only using about 135px.

So I went with the same position property checks that the other tests use.

@@ -364,7 +376,8 @@
* Scrolls the list to the given Date.
*/
scrollToDate(date, animate) {
this._scrollToPosition(this._differenceInMonths(date, this._originDate), animate);
const offset = this.__useSubMonthScrolling ? this._calculateWeekScrollOffset(date) : 0;
this._scrollToPosition(this._differenceInMonths(date, this._originDate) + offset, animate);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This covers the initial scrolling to the focused date when opening the overlay.

Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Tested manually, seems to work fine 👍

@sissbruecker
Copy link
Contributor Author

Tested a bit more and found one issue. When opening the overlay for the first time, the month below the one that is currently selected briefly displays the wrong month name. In the video below the 13th of October is selected, the month below should show "November", but is briefly displayed as "October":

Bildschirmaufnahme.2022-10-13.um.17.58.02.mov

(might have to use the scrubber to find the correct frames, it's more noticable when testing live)

@sissbruecker
Copy link
Contributor Author

When opening the overlay for the first time, the month below the one that is currently selected briefly displays the wrong month name.

Added a commit that forces the infinite scroller to update when opening the overlay.

@sissbruecker
Copy link
Contributor Author

After a discussion we also decided to reduce the month scroll offset (--vaadin-infinite-scroller-buffer-offset), so that a single month takes up less space, and the week-based scrolling is enabled as late as possible.

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.

4 participants