feat(menu-surface): add component #295
Conversation
…terial-components-web-react into feat/menu-surface
| @@ -0,0 +1,149 @@ | |||
| # React Menu Surface | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #295 +/- ##
==========================================
+ Coverage 97.14% 97.24% +0.09%
==========================================
Files 35 36 +1
Lines 1367 1489 +122
Branches 131 163 +32
==========================================
+ Hits 1328 1448 +120
- Misses 39 41 +2
Continue to review full report at Codecov.
|
moog16
changed the title
| window.removeEventListener('contextmenu', this.rightClickCallback_); | ||
| } | ||
|
|
||
| setAnchorElement = (element) => { |
There was a problem hiding this comment.
Why update state here instead of using createRef?
There was a problem hiding this comment.
I want the component to re-render when the ref is set. I put it on state, so that it renders again once it sets. Otherwise I did a forceRender(), which isn't as obvious.
packages/menu-surface/index.js
Outdated
| } | ||
|
|
||
| hoistToBody() { | ||
| // this deviatees from the mdc web version. |
| // this deviatees from the mdc web version. | ||
| // here we force the menu to hoist, and require either | ||
| // this.props.(x,y) or this.props.anchorElement. | ||
| if (!(this.menuSurfaceElement_ && this.menuSurfaceElement_.current)) return; |
There was a problem hiding this comment.
I think we can remove the null check for this.menuSurfaceElement_
There was a problem hiding this comment.
I think its safer to keep it in. adapter.restoreFocus is called within foundation.close(). foundation.close is called in handleBodyClick and handleKeydown, which are async events. There is the possibility that the MenuSurface is unmounted before a keydown event or a click event occurs, which would cause an error.
What do you think?
|
I think its the same I attached screenshots of web and react both set to BOTTOM_END
|
| removeMenuFromBody(wrapper); | ||
| }); | ||
|
|
||
| test('update to props.open sets firstFocusableElement_', () => { |
|
Oh one thing I noticed was that the menu surface doesn't trap focus like the MDC Web one. We can either fix that in this PR or open a new bug for it |
|
@bonniezhou I don't think menu-surface traps focus. Menu does - is that what you meant? |
|
Oh it might have been menu that I was looking at. Disregard! :) |
fixes #188