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 month is visible when receiving focus (#3219) (CP: 14.0) #799

Merged
merged 4 commits into from
May 10, 2022

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Jan 7, 2022

Description

A cherry-pick of vaadin/web-components#3219.

Fixes vaadin/web-components#1892.

Type of change

  • Bugfix

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2022

CLA assistant check
All committers have signed the CLA.

@vursen vursen requested a review from tomivirkki January 7, 2022 14:13
@vursen vursen force-pushed the cp/ensure-focused-month-is-visible branch 2 times, most recently from 9c48caf to d2b353f Compare January 9, 2022 19:42
@vursen
Copy link
Contributor Author

vursen commented Jan 10, 2022

Some newly added unit tests are failing in IE11. Debugging showed that clientHeight of the month scroller in IE11 is different from other browsers. It may have something to do with the flex layout:

https://stackoverflow.com/questions/58175011/ie11-has-different-clientwidth-and-clientheight-than-all-other-browsers

@web-padawan web-padawan force-pushed the cp/ensure-focused-month-is-visible branch from 75506a1 to 3e5f6d1 Compare January 24, 2022 15:02
@yuriy-fix
Copy link
Contributor

Closing as the original fix is not aiming critical issue. No need to be backported.

@yuriy-fix yuriy-fix closed this Jan 28, 2022
@vursen
Copy link
Contributor Author

vursen commented Jan 28, 2022

Closing as the original fix is not aiming critical issue. No need to be backported.

Let me not agree. Were the following factors considered before closing?

  • The original issue was reported for V14.
  • The scrolling error accumulates in certain conditions so that at some point you may end up navigating completely outside the visible area without this fix.

@vursen vursen reopened this May 9, 2022
@vursen vursen force-pushed the cp/ensure-focused-month-is-visible branch 3 times, most recently from 9eeea1b to b4cc6e5 Compare May 9, 2022 13:27
@vursen vursen force-pushed the cp/ensure-focused-month-is-visible branch from b4cc6e5 to 8a44825 Compare May 9, 2022 13:58
@vursen
Copy link
Contributor Author

vursen commented May 9, 2022

For some reason, Gemini demanded to update the week-number screenshot even though there is no visible difference: 53d66d2. At least, I couldn't spot any difference whatever angle I looked at it from 🤔

I assume, however, there can be a very subtle difference in scroll position that can be a result of the revealDate method that was changed in this PR.

Copy link
Member

@tomivirkki tomivirkki left a comment

Choose a reason for hiding this comment

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

Works well, also on IE11

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.

[date-picker] The focus-ring on the focused day is getting hidden while pressing Arrow Down
4 participants