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

Select option list follows page scroll #1500

Merged
merged 13 commits into from
Nov 20, 2017
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
"karma-coverage": "^1.1.0",
"karma-firefox-launcher": "^1.0.0",
"karma-mocha": "^1.3.0",
"karma-sauce-launcher": "^1.0.0",
"karma-sauce-launcher": "^1.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Take this out of this PR

"karma-sourcemap-loader": "^0.3.7",
"karma-tap": "^2.1.4",
"karma-webpack": "^2.0.4",
Expand Down
1 change: 1 addition & 0 deletions packages/mdc-button/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@
-webkit-appearance: none;
overflow: hidden;
vertical-align: middle;
-webkit-tap-highlight-color: rgba(0, 0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

And remove this from this PR


// postcss-bem-linter: ignore
&:active {
Expand Down
2 changes: 1 addition & 1 deletion packages/mdc-checkbox/_mixins.scss
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,8 @@
&-checked-unchecked,
&-indeterminate-unchecked {
.mdc-checkbox__background {
animation-duration: $mdc-checkbox-transition-duration * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

this changes seems to have sneaked into this PR...can you make sure packages/mdc-checkbox/_mixins.scss is the exact same as our master branch?

animation-timing-function: linear;
animation-duration: $mdc-checkbox-transition-duration * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

And remove this from this PR

}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/mdc-select/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,8 @@ within `componentDidUpdate`.
| --- | --- |
| `addClass(className: string) => void` | Adds a class to the root element. |
| `removeClass(className: string) => void` | Removes a class from the root element. |
| `addBodyClass(className: string) => void` | Adds a class to the body. |
| `removeBodyClass(className: string) => void` | Removes a class from the body. |
| `setAttr(attr: string, value: string) => void` | Sets attribute `attr` to value `value` on the root element. |
| `rmAttr(attr: string) => void` | Removes attribute `attr` from the root element. |
| `computeBoundingRect() => {left: number, top: number}` | Returns an object with a shape similar to a `ClientRect` object, with a `left` and `top` property specifying the element's position on the page relative to the viewport. The easiest way to achieve this is by calling `getBoundingClientRect()` on the root element. |
Expand Down
1 change: 1 addition & 0 deletions packages/mdc-select/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export const cssClasses = {
ROOT: 'mdc-select',
OPEN: 'mdc-select--open',
DISABLED: 'mdc-select--disabled',
SCROLL_LOCK: 'mdc-select-scroll-lock',
};

export const strings = {
Expand Down
12 changes: 12 additions & 0 deletions packages/mdc-select/foundation.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ export default class MDCSelectFoundation extends MDCFoundation {
return {
addClass: (/* className: string */) => {},
removeClass: (/* className: string */) => {},
addBodyClass: (/* className: string */) => {},
removeBodyClass: (/* className: string */) => {},
setAttr: (/* attr: string, value: string */) => {},
rmAttr: (/* attr: string */) => {},
computeBoundingRect: () => /* {left: number, top: number} */ ({left: 0, top: 0}),
Expand Down Expand Up @@ -183,6 +185,7 @@ export default class MDCSelectFoundation extends MDCFoundation {
}

open_() {
this.disableScroll_();
const {OPEN} = MDCSelectFoundation.cssClasses;
const focusIndex = this.selectedIndex_ < 0 ? 0 : this.selectedIndex_;

Expand Down Expand Up @@ -220,6 +223,7 @@ export default class MDCSelectFoundation extends MDCFoundation {
const {OPEN} = MDCSelectFoundation.cssClasses;
this.adapter_.removeClass(OPEN);
this.adapter_.focus();
this.enableScroll_();
}

handleDisplayViaKeyboard_(evt) {
Expand All @@ -243,5 +247,13 @@ export default class MDCSelectFoundation extends MDCFoundation {
this.displayHandler_(evt);
}
}

disableScroll_() {
this.adapter_.addBodyClass(cssClasses.SCROLL_LOCK);
}

enableScroll_() {
this.adapter_.removeBodyClass(cssClasses.SCROLL_LOCK);
}
}

2 changes: 2 additions & 0 deletions packages/mdc-select/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ export class MDCSelect extends MDCComponent {
deregisterMenuInteractionHandler: (type, handler) => this.menu_.unlisten(type, handler),
notifyChange: () => this.emit(MDCSelectFoundation.strings.CHANGE_EVENT, this),
getWindowInnerHeight: () => window.innerHeight,
addBodyClass: (className) => document.body.classList.add(className),
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you add this class to the document body? Why can't this class be added directly to the this.root_ element?

Copy link
Contributor

Choose a reason for hiding this comment

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

He is looking at the same implementation for the drawer and doing the same here 😃

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the context @touficbatache

removeBodyClass: (className) => document.body.classList.remove(className),
});
}

Expand Down
5 changes: 5 additions & 0 deletions packages/mdc-select/mdc-select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -264,3 +264,8 @@
// with CSS. Use the font-size rule instead.

// postcss-bem-linter: end

.mdc-select-scroll-lock {
height: 100vh;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to set the hight? Is overflow: hidden not enough to remove the scroll bar?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just like that in the drawer implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

I did some digging into the drawer (and dialog for that matter) and we're 99% sure that height: 100vh is unnecessary.

So, @roncapat, go ahead and remove it here too. (#1610 and #1612 for the changes we're making to drawer and dialog)

overflow: hidden;
}
6 changes: 3 additions & 3 deletions packages/mdc-theme/_color-palette.scss
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ $material-color-purple-900: #4a148c;
$material-color-purple-a100: #ea80fc;
$material-color-purple-a200: #e040fb;
$material-color-purple-a400: #d500f9;
$material-color-purple-a700: #aa00ff;
$material-color-purple-a700: #a0f;
Copy link
Contributor

Choose a reason for hiding this comment

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

And remove this from this PR


$material-color-deep-purple-50: #ede7f6;
$material-color-deep-purple-100: #d1c4e9;
Expand Down Expand Up @@ -207,7 +207,7 @@ $material-color-yellow-700: #fbc02d;
$material-color-yellow-800: #f9a825;
$material-color-yellow-900: #f57f17;
$material-color-yellow-a100: #ffff8d;
$material-color-yellow-a200: #ffff00;
$material-color-yellow-a200: #ff0;
Copy link
Contributor

Choose a reason for hiding this comment

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

this changes seems to have sneaked into this PR...can you make sure packages/mdc-theme/_color-palette.scss is the exact same as our master branch?

$material-color-yellow-a400: #ffea00;
$material-color-yellow-a700: #ffd600;

Expand Down Expand Up @@ -269,7 +269,7 @@ $material-color-brown-900: #3e2723;

$material-color-grey-50: #fafafa;
$material-color-grey-100: #f5f5f5;
$material-color-grey-200: #eeeeee;
$material-color-grey-200: #eee;
$material-color-grey-300: #e0e0e0;
$material-color-grey-400: #bdbdbd;
$material-color-grey-500: #9e9e9e;
Expand Down
2 changes: 1 addition & 1 deletion test/unit/mdc-select/foundation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ test('default adapter returns a complete adapter implementation', () => {
'isMenuOpen', 'setSelectedTextContent', 'getNumberOfOptions', 'getTextForOptionAtIndex',
'setAttrForOptionAtIndex', 'rmAttrForOptionAtIndex', 'getOffsetTopForOptionAtIndex',
'registerMenuInteractionHandler', 'deregisterMenuInteractionHandler', 'notifyChange',
'getWindowInnerHeight', 'getValueForOptionAtIndex',
'getWindowInnerHeight', 'getValueForOptionAtIndex', 'addBodyClass', 'removeBodyClass',
]);

const renderingContext = MDCSelectFoundation.defaultAdapter.create2dRenderingContext();
Expand Down
15 changes: 15 additions & 0 deletions test/unit/mdc-select/mdc-select.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,3 +379,18 @@ test('adapter#getValueForOptionAtIndex returns the textContent of the option at
const {component} = setupTest();
assert.equal(component.getDefaultFoundation().adapter_.getValueForOptionAtIndex(3), 'Item 4 no id');
});

test('adapter#addBodyClass adds a class to the body', () => {
const {component} = setupTest();
component.getDefaultFoundation().adapter_.addBodyClass('mdc-select-scroll-lock');
assert.isOk(document.querySelector('body').classList.contains('mdc-select-scroll-lock'));
});

test('adapter#removeBodyClass remove a class from the body', () => {
const {component} = setupTest();
const body = document.querySelector('body');

body.classList.add('mdc-select-scroll-lock');
component.getDefaultFoundation().adapter_.removeBodyClass('mdc-select-scroll-lock');
assert.isNotOk(body.classList.contains('mdc-select-scroll-lock'));
});