[For Review] Menu/Popover/IconMenu/Dialog Portal updates by chrismcv · Pull Request #1845 · mui/material-ui

chrismcv · 2015-10-08T23:20:44Z

This PR
a) currently breaks dialogs
b) introduces a component Popover which is an arbitrary control that shows a popup menu, either onClick or on props.open = true.
c) Popover component wraps menu in IconMenu - nearly transparently.
d) Popover renders to a new DOM Tree, and in theory context should transfer for React@0.13
e) Dialog renders into new tree, but has issues that I'll sort shortly...

patrykkopycinski · 2015-10-09T06:13:17Z

docs/src/app/app-routes.jsx

is it necessary?

chrismcv · 2015-10-09T11:53:02Z

Updated PR:

Dialog is fixed via a wrapper
I'm having a problem with warnings on the DatePickerDialog/TimePickerDialog and contexts - any suggestions welcome @hai-cea @shaurya947
I think there is some scope to simplify menu's now that animation/positioning is delegated to the popover component

chrismcv · 2015-10-15T15:38:26Z

I've updated this PR - lots of changes....

New component "Popover"
New mixin "Render to layer"
React 14 support

Updated the following components to use it

Dropdown menu
Icon Menu
Dialog
SelectField
Menu (menus/menu-item.jsx) when it has nested items

It'd be great if review + merge process could start?

oliviertassinari · 2015-10-26T19:18:24Z

src/text-field.jsx

Could you remove the extra commit from #1896?

#1896 is that single commit... do you want it removed from #1845 (the big one?)

I think that #1896 will be reviewed and merged before this big PR.
I don't know if github will complain if we merge #1896 first.

shaurya947 · 2015-10-28T17:02:01Z

@chrismcv are you still having the warnings regarding context? I was hoping they would go away with React v14 as context moves from being owner-based to parent-based.

@oliviertassinari seems like you have reviewed this a little. Is it merge-ready?

chrismcv · 2015-10-28T17:08:41Z

v14 has sorted context issues happily. I removed the initial context hacks that I had in for v13.

I think the changes here are significant, and technically, they are just about there now.

There are a few more docs changes I think we should put in. (And at least one fix to docs site "customisation section").

I've made dialog have an open property which controls visibility and depreciates show and hide. There is a manual override that allows show and hide but this is defaulted to false which means a breaking change.

The Dropdown/Icon Menu transitions are subtly different from previous.

The bit I'd like you to look at is the interface for position the popover. I've introduced anchorEl and targetEl positions. So you can say that the top left of the popover should show originating at the bottom right of the target element. (see icon menu docs for examples).

My nested menu support is a little rudimentary, its currently missing some keyboard support - but this seems partly true of the new menus stuff in general.

shaurya947 · 2015-10-28T17:22:19Z

@chrismcv look at #1996 regarding props for Dialog.

oliviertassinari · 2015-10-28T17:54:07Z

@shaurya947 not yet

chrismcv · 2015-10-28T18:36:50Z

@shaurya947 I've landed on something very similar to #1996, bar my property is called open rather than isOpen and I haven't removed the imperative API, though am happy to do that? Was that the final agreement for #1996?

shaurya947 · 2015-10-28T18:52:13Z

@chrismcv we're slightly short of a final agreement on that. Should reach a conclusion soon hopefully! :-)

oliviertassinari · 2015-11-02T11:44:14Z

docs/src/app/components/pages/components/popover.jsx

Do we need those imperative methods? I would rather avoid them.

oliviertassinari · 2015-11-02T11:52:21Z

@chrismcv There is a lot in this PR. Can't we split this into independent bit?

chrismcv · 2015-11-02T11:56:54Z

@oliviertassinari - yes I agree.
Happy to split - and can do so this evening I think.

Does the following make sense (1+2 being dependencies for 3+4)

Render to Layer component
Popover component
Dialog to use render to layer (and fix other dialogs + docs)
IconMenu/NestedMenus

shaurya947 · 2015-11-10T21:16:40Z

@chrismcv is this PR stale then? Are you going to close it?

chrismcv · 2015-11-11T10:16:38Z

closing for now - more to follow

* Change component name from MuiPickers to MuiPicker * Name root classnames "root" * Use pseudo-classes for disabled styling * Remove useless yearDisabled class * Update demo and typescript tests for overrides * Repalce MuiPicker with MuiPickers * Consolidate right component display name with mui component name * Update index.ts reexports to match component display names * Remove useless parameter * Fix docs example * Fix override example tsc * Use pseudo-classes for selected and disabled, closes mui#1845 * Address PR feedback * Update lib/src/Picker/Picker.tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * Update lib/src/views/Clock/Clock.tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> * Make more convenient css classes names * Remove today border from selected day * Remove useless comment * Update tests * Fix global overrid of Mui-selected and Mui-disabled classes * Use theme.palette.secondary instead of hint * Fix linter * Fix incorrect package prefix Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>

patrykkopycinski reviewed Oct 9, 2015
View reviewed changes

docs/src/app/app-routes.jsx Outdated

Copy link

Contributor

patrykkopycinski Oct 9, 2015

Choose a reason for hiding this comment

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

is it necessary?

chrismcv force-pushed the popups branch from 7f4e39e to 0b2d610 Compare October 9, 2015 11:42

chrismcv changed the title ~~[For Review] Menu updates~~ [For Review] Menu/Popover/IconMenu/Dialog Portal updates Oct 9, 2015

This was referenced Oct 9, 2015

Question: How to use material-ui react components with the Portal Pattern? #1675

Closed

[Menus] - z-index/layering/document placement #1621

Closed

chrismcv force-pushed the popups branch 6 times, most recently from b7ff8ac to 201dfc1 Compare October 15, 2015 15:35

chrismcv force-pushed the popups branch 5 times, most recently from 158f189 to d3d46d1 Compare October 21, 2015 15:20

chrismcv force-pushed the popups branch from d3d46d1 to 70fa4a8 Compare October 26, 2015 10:03

oliviertassinari added the GH Review: review-needed label Oct 26, 2015

oliviertassinari reviewed Oct 26, 2015
View reviewed changes

chrismcv force-pushed the popups branch from 70fa4a8 to 0c01421 Compare October 26, 2015 20:56

chrismcv mentioned this pull request Oct 26, 2015

text field style fix #1896

Merged

chrismcv force-pushed the popups branch from 0c01421 to 9884091 Compare October 27, 2015 23:13

chrismcv mentioned this pull request Oct 28, 2015

Stacked event listeners #1533

Closed

chrismcv force-pushed the popups branch from 4e766d2 to 4f89d09 Compare November 2, 2015 09:30

oliviertassinari reviewed Nov 2, 2015
View reviewed changes

docs/src/app/components/pages/components/popover.jsx

Copy link

Member

oliviertassinari Nov 2, 2015

Choose a reason for hiding this comment

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

Do we need those imperative methods? I would rather avoid them.

This was referenced Nov 2, 2015

Render to Layer component #2042

Merged

Popover #2043

Merged

chrismcv added 2 commits November 3, 2015 11:18

Menu/Popover/IconMenu/Dialog Portal updates

0135017

change render-to-layer to click away on mousedown

3058f8b

chrismcv force-pushed the popups branch from 4f89d09 to 3058f8b Compare November 3, 2015 11:18

chrismcv mentioned this pull request Nov 6, 2015

Allow open state of dialog to be controlled through the show prop. #1996

Merged

dropdown fix

74ade4e

chrismcv closed this Nov 11, 2015

This was referenced Nov 11, 2015

dialog render-to-layer version #2129

Merged

Menu has nested items support using popover #2148

Merged

icon-menu using popover #2149

Merged

dropdown menu using popover #2150

Merged

alitaheri removed the PR: Needs Review label Feb 15, 2016

zannager added scope: dialog Changes related to the dialog. scope: menu Changes related to the menu. component: Popover The React component. labels Mar 14, 2023

Uh oh!

Conversation

chrismcv commented Oct 8, 2015

Uh oh!

patrykkopycinski Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

chrismcv commented Oct 9, 2015

Uh oh!

chrismcv commented Oct 15, 2015

Uh oh!

oliviertassinari Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

chrismcv Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

shaurya947 commented Oct 28, 2015

Uh oh!

chrismcv commented Oct 28, 2015

Uh oh!

shaurya947 commented Oct 28, 2015

Uh oh!

oliviertassinari commented Oct 28, 2015

Uh oh!

chrismcv commented Oct 28, 2015

Uh oh!

shaurya947 commented Oct 28, 2015

Uh oh!

oliviertassinari Nov 2, 2015

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Nov 2, 2015

Uh oh!

chrismcv commented Nov 2, 2015

Uh oh!

shaurya947 commented Nov 10, 2015

Uh oh!

chrismcv commented Nov 11, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants