feat(menu-surface): Update setPosition adapter API to use numeric values #4351
Conversation
Accepting `string` values implies that we support all valid CSS units (`em`, `rem`, `%`, etc.), but in truth we only support `px`. Changing the type of the properties from `string` to `number` makes this clearer, and also simplifies converting the package to TypeScript in PR #4273. BREAKING CHANGE: `MDCMenuSurfaceAdapter.setPosition()` now expects an object with properties of type `number` rather than `string`. E.g., `setPosition({top: '5px', left: '10px'})` is now `setPosition({top: 5, left: 10})`.
| this.root_.style.right = 'right' in position ? position.right : null; | ||
| this.root_.style.top = 'top' in position ? position.top : null; | ||
| this.root_.style.bottom = 'bottom' in position ? position.bottom : null; | ||
| this.root_.style.left = 'left' in position ? `${position.left}px` : ''; |
There was a problem hiding this comment.
Why did this change from null to '' here, if we're not doing that in the TypeScript PR? (It occurred to me to suggest this there, but I figured we were preserving existing behavior.)
There was a problem hiding this comment.
The Closure Compiler test suddenly complained that these values can't be null 🤷♂️
TypeScript's lib.dom.d.ts, however, includes | null in the types for all of these properties, so tsc doesn't complain about it.
There was a problem hiding this comment.
CSS specs seem to indicate that null is not a valid value. Empty string is preferred.
So Closure Compiler is technically correct; TypeScript is wrong.
-
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/setProperty
valueOptional is aDOMStringcontaining the new property value. If not specified, treated as the empty string. -
https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration/getPropertyValue
Return value is a
DOMStringcontaining the value of the property. If not set, returns the empty string. -
https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface
-
https://heycam.github.io/webidl/#idl-DOMString
Note: Note also that
nullis not a value of typeDOMString. To allownull, a nullableDOMString, written asDOMString?in IDL, needs to be used.
Codecov Report
@@ Coverage Diff @@
## master #4351 +/- ##
==========================================
+ Coverage 98.48% 98.56% +0.08%
==========================================
Files 130 130
Lines 5736 5731 -5
Branches 766 763 -3
==========================================
Hits 5649 5649
+ Misses 87 82 -5
Continue to review full report at Codecov.
|
|
All 621 screenshot tests passed for commit 08f5fe9 vs. |
|
All 621 screenshot tests passed for commit 5264ea9 vs. |
|
All 621 screenshot tests passed for commit acfef88 vs. |
|
All 621 screenshot tests passed for commit 8501b4d vs. |
| @@ -26,7 +26,7 @@ | |||
| * top: number, | |||
| * right: number, | |||
| * bottom: number, | |||
| * left: number | |||
| * left: number, | |||
There was a problem hiding this comment.
Is trailing comma valid in closure annotations?
There was a problem hiding this comment.
It's Closure, so probably not. Removed. Thanks!
|
All 621 screenshot tests passed for commit fb02775 vs. |
…al-components/material-components-web into feat/menu-surface/setPosition-api
|
All 621 screenshot tests passed for commit 55ac7cf vs. |
Accepting
stringvalues implies that we support all valid CSS units (em,rem,%, etc.), but in truth we only supportpx.Changing the type of the properties from
stringtonumbermakes this clearer, and also simplifies converting the package to TypeScript in PR #4273.BREAKING CHANGE:
MDCMenuSurfaceAdapter.setPosition()now expects an object with properties of typenumberrather thanstring. E.g.,setPosition({top: '5px', left: '10px'})is nowsetPosition({top: 5, left: 10}).