-
Notifications
You must be signed in to change notification settings - Fork 273
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(ui5-datetime-picker): fix scrollbar issue in IE11 #2154
Conversation
packages/main/src/DateTimePicker.js
Outdated
@@ -250,6 +251,9 @@ class DateTimePicker extends DatePicker { | |||
this.expandHoursSlider(); | |||
this.storePreviousValue(); | |||
this._slidersDomRefs = await this.slidersDomRefs(); | |||
if (isIE()) { | |||
(await this.getPicker()).shadowRoot.querySelector(".ui5-popup-content").style.overflow="hidden"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we add styles in different way, more declarative as we make use of the templates.
Example:
// DatePicker.js
get styles() {
return {
main: {
width: "100%",
},
};
}
<!-- DatePicker.hbs -->
<div
style="{{styles.main}}"
>
And second, .ui5-popup-content element belongs to the Popover and it is too rough to apply styles from the DateTimePicker.
I suggest adding a private (in sense of protected) property in the Popover.js that defines if overflow:hidden should be added. And then set this property to the "<ui5-responsive-popover" within DatePicker.hbs (the template is reused for the DateTimePicker) in context if IE.
In addition to the DateTimePicker, test if everything looks good for the DatePicker as well.
How to name the property? It defines whether the content is scrollable. Also, we should have in mind that all Boolean props in HTML are "false" by default. Then it should be the opposite of scrollable and contentScrollable (because this should be the default state). So, I guess "disableScrolling" is fine.
// Popover.js
/**
* Defines whether the content is scrollable.
*
* @type {boolean}
* @defaultvalue false
* @private
*/
disableScrolling: {
type: Boolean,
},
// PopupsCommon.css
:host([disable-scrolling]) .ui5-popup-content {
overflow: hidden;
}
// DatePicker.js
get _isIE() {
return isIE();
}
<!-- DatePickerPopover.hbs -->
<ui5-responsive-popover
?disable-scrolling="{{_isIE}}"
no-arrow
with-padding
no-stretch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanx, fixed as requested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference the issue the PR resolves in the description: FIXES <ISSUE_URL>
* DateTimePicker - fixed scrollbar issue in IE11 * lint fix * fixing comments * fixing comments
* DateTimePicker - fixed scrollbar issue in IE11 * lint fix * fixing comments * fixing comments
Fixes: #2012