[Dialog] create responsive HOC `withResponsiveFullScreen` #6898

rosskevin · 2017-05-18T20:17:04Z

The spec discusses that Dialogs should be full screen on mobile. As such, I have defaulted the Dialog to be responsively fullScreen at or below the sm breakpoint.

You may specify a different breakpoint or force it fullScreen with true.

Other changes:

flow - configured suppression comments and converted a few more components
mocha/package.json - specifying file patterns in mocha.opts means that all these files are processed every time - good for CI, really bad when you are trying to debug a single test. I have moved the spec file patterns back to the scripts section in package.json accordingly.

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

oliviertassinari

Some early feedback (haven't look in detail).
Also, I'm not convinced about the new default behaviour of the Dialog. Taking my Android phone, there is many screens where the Dialog isn't full screen on mobile.

oliviertassinari · 2017-05-18T20:35:07Z

src/Dialog/Dialog.js

@@ -1,14 +1,15 @@
 // @flow weak

-import React, { Component } from 'react';
-import PropTypes from 'prop-types';
+import React, { PureComponent, Element } from 'react';


I don't think that we need any pure logic inside Material-UI, our components are too low level to be efficient.

oliviertassinari · 2017-05-18T20:36:17Z

src/Dialog/Dialog.js

@@ -46,6 +47,111 @@ export const styleSheet = createStyleSheet('MuiDialog', (theme) => {
  };
 });

+type MaxWidthBreakpoints = 'xs' | 'sm' | 'md';


To verify but I don't think that https://github.com/brigand/babel-plugin-flow-react-proptypes can handle the indirection resulting in .any.

oliviertassinari · 2017-05-18T20:40:29Z

really bad when you are trying to debug a single test.

You can use the .only helper, actually, I'm using that everytime.

oliviertassinari · 2017-05-18T20:45:48Z

From my perspective, the fullscreen mode is an escape hatch for people looking for a quick way to implement that on mobile. That's why I'm not 100% regarding promoting it as a default. From my work on the mobile version of https://www.doctolib.fr/, I would definitely not use it when implementing transitions between my routes or even for the layout of my screens. Still, I understand that it's quite useful!

oliviertassinari

Did an in-depth review. Think that we have the opportunity to build a great Higher-order Component with that breakpoint concern 😄 . I'm happy to see that concern raised! Also, great to see more propTypes migrated to flow.

oliviertassinari · 2017-05-19T07:13:09Z

docs/src/pages/component-demos/dialogs/dialogs.md

-## Full-screen dialogs
+## Responsive full-screen dialogs
+
+By default, dialogs are responsively full screened *at or below* the `sm` [screen size](/layout/basics).  You may alter the breakpoint or force it to be full screen at all times as shown here.


Double spaces?

oliviertassinari · 2017-05-19T07:13:54Z

src/Dialog/Dialog.js

+   */
+  children?: Element<*>,
+  /**
+   * The CSS class name of the root element.


What do you think about using @ignore? This is quite idiomatic and standard in the ecosystem. Why documenting it?

oliviertassinari · 2017-05-19T07:16:33Z

src/Dialog/Dialog.js

-  static defaultProps = {
-    fullScreen: false,
+  static defaultProps: DefaultProps = {
+    fullScreen: 'sm',


What do you think of the UI? I'm not sure it's in the specification.

Also, this default value isn't SSR compliant.

oliviertassinari · 2017-05-19T07:20:30Z

src/Dialog/Dialog.js

@@ -237,3 +265,5 @@ export default class Dialog extends Component {
    );
  }
 }
+
+export default withWidth()(Dialog);


What about using a decorator instead? Something that we could be using to switch properties off and on based on the breakpoint? I see many use cases for that new Higher-order Component.

rosskevin · 2017-05-19T15:07:47Z

Ok, I was trying to be incremental @oliviertassinari ;), but now is as good of a time as any to review and revise.

Types of Dialogs:

Alert - requires button press (no backdrop click listener for dismissal), not fullScreen.
Simple - no cancelation, no buttons, not fullScreen
Confirmation - requires cancel/ok, not fullScreen
FullScreen - is fullScreen!

Responsive

So the fullScreen spec cases do look very intentional, which makes me think the responsiveDialog HOC makes a great deal of sense, that's a good suggestion.

Back to types

It seems like it makes sense for /Dialog to export at least a spec compliant Alert (with ignoreBackdropClick: true and perhaps others as-needed if we spot something in the spec.

I'll start revising.

oliviertassinari · 2017-05-19T17:59:03Z

src/Dialog/withResponsiveFullScreen.js

+ * Dialog will responsively be full screen _at or below_ the given breakpoint
+ * (defaults to 'sm' for mobile devices).
+ */
+function withResponsiveFullScreen(options: Options = { breakpoint: 'sm' }) {


Yeah, I like this Higher-order Component ❤️ . We could imagine making it even more generic with a property modifier function. Some idead I have in mind. I'm not saying it should be like this.

import Dialog from 'material-ui/Dialog' import withResponsiveProps from 'material-ui/utils/withResponsiveProps' const ResponsiveDialog = withResponsiveProps({ breakpoints: 'md', switch: (props, match) => ({ fullscrean: match ? true : false, ...props, }) })(Dialog)

or

import Dialog from 'material-ui/Dialog' import ResponsiveProps from 'material-ui/utils/withResponsiveProps' <ResponsiveProps breakpoints="md" switch={(props, match) => ({ fullscrean: match ? true : false, })} > <Dialog /> </ResponsiveProps>

Still, I think that we can merge that HOC as already providing value and elegant 😄 .

oliviertassinari · 2017-05-19T18:02:07Z

A rebase would be great, I'm a bit lost in the diff. Aside from that, I don't have much feedback. I think that we are going in the right direction 😄 .

rosskevin · 2017-05-19T20:50:34Z

The only with the solution up is that you have to think about only | isWidthUp | isWidthDown. While it might make sense to have something internal, I think the external facing usages need to be concrete. The future may prove me wrong though.

I'm going to get this rebase figured out.

…r on startup

rosskevin · 2017-05-19T22:15:56Z

I'm working on spec Alert vs Confirmation vs Simple Dialog in another PR, but it will be branched from this.

Review items implemented.

oliviertassinari · 2017-05-20T06:37:27Z

test/mocha.opts

@@ -2,5 +2,3 @@
 --reporter dot
 --recursive
 test/utils/setup.js
-test/integration/{,**/}*.spec.js


Did you try the only thing? Now we have that config at three different locations, not great.

I use a jetbrains IDE, and that doesn't seem to be an option. Up to this point I have been deleting the offending code and avoiding committing it. While I very much dislike duplication, it seems necessary here.

oliviertassinari · 2017-05-20T06:40:16Z

src/transitions/Fade.js

+  onExited?: Function, // eslint-disable-line react/sort-prop-types
+};
+
+export default class Fade extends PureComponent<DefaultProps, Props, void> {


As a rule of thumb, we should not use any pure logic with components accepting react elements. They are created at each render, making strict comparison always negative (in some rare cases true when the element is hoisted outside of the render method), hence, wasted CPU cycles.

This was a Component as was the other you mentioned, so you could say this is an improvement over there former. This time there is quite more in the body of the class where I much prefer class closure from a source perspective. Let's be a bit incremental about the codebase, I unfortunately don't always have time to take the former code and make every PR exactly inline. Each PR author needs to be able to improve the code an increment without taking on a burden of everything that might be wrong with a file.

oliviertassinari · 2017-05-20T06:41:34Z

src/Dialog/withResponsiveFullScreen.js

+ * Dialog will responsively be full screen _at or below_ the given breakpoint
+ * (defaults to 'sm' for mobile devices).
+ */
+function withResponsiveFullScreen(options: Options = { breakpoint: 'sm' }) {


Still, I think that we can merge that HOC as already providing value and elegant 😄 .

oliviertassinari · 2017-05-20T06:42:37Z

src/Dialog/Dialog.js

@@ -46,6 +45,92 @@ export const styleSheet = createStyleSheet('MuiDialog', (theme) => {
  };
 });

+type Props = {


Hold on with the flow migration, I'm refactoring all the components to use withStyles. Let's try to avoid conflict 🙈 .

I'm going to merge this as I've already had a rebase nightmare with it. Will hold off on more, and we can discuss any other things that need to be done on gutter and I'll get to it on Monday. Thanks for all the feedback, all good things and this code is better for it!

rosskevin added component: dialog This is the name of the generic UI component, not the React module! next labels May 18, 2017

muibot added the PR: Needs Review label May 18, 2017

oliviertassinari requested changes May 18, 2017

View reviewed changes

oliviertassinari previously requested changes May 19, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label May 19, 2017

muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 19, 2017

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels May 19, 2017

rosskevin changed the title ~~[Dialog] make fullScreen responsive~~ WIP [Dialog] make fullScreen responsive May 19, 2017

oliviertassinari reviewed May 19, 2017

View reviewed changes

rosskevin added 4 commits May 19, 2017 16:07

[flow] type Dialog, Modal, Backdrop, Fade

4c55fee

[mocha] move file patterns out of mocha.opts as it slows down debugge…

06c6a26

…r on startup

[Dialog] implement HOC withResponsiveFullScreen

d6e424f

[Dialog] docs for HOC

70c2301

rosskevin force-pushed the responsive-dialog branch from 7dfb400 to 70c2301 Compare May 19, 2017 21:10

rosskevin changed the title ~~WIP [Dialog] make fullScreen responsive~~ [Dialog] create responsive HOC withResponsiveFullScreen May 19, 2017

rosskevin removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label May 19, 2017

rosskevin mentioned this pull request May 19, 2017

[Dialog] disambiguation of types to match the spec #6903

Closed

3 tasks

oliviertassinari reviewed May 20, 2017

View reviewed changes

rosskevin merged commit e004435 into mui:next May 20, 2017

rosskevin deleted the responsive-dialog branch June 14, 2017 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dialog] create responsive HOC `withResponsiveFullScreen` #6898

[Dialog] create responsive HOC `withResponsiveFullScreen` #6898

rosskevin commented May 18, 2017 •

edited

oliviertassinari left a comment •

edited

oliviertassinari May 18, 2017

oliviertassinari May 18, 2017

oliviertassinari commented May 18, 2017 •

edited

oliviertassinari commented May 18, 2017

oliviertassinari left a comment

oliviertassinari May 19, 2017 •

edited

oliviertassinari May 19, 2017

oliviertassinari May 19, 2017

oliviertassinari May 19, 2017

oliviertassinari May 19, 2017

rosskevin commented May 19, 2017

oliviertassinari May 19, 2017

oliviertassinari May 20, 2017

oliviertassinari commented May 19, 2017

rosskevin commented May 19, 2017

rosskevin commented May 19, 2017

oliviertassinari May 20, 2017 •

edited

rosskevin May 20, 2017

oliviertassinari May 20, 2017 •

edited

rosskevin May 20, 2017

oliviertassinari May 20, 2017

oliviertassinari May 20, 2017

rosskevin May 20, 2017

[Dialog] create responsive HOC withResponsiveFullScreen #6898

[Dialog] create responsive HOC withResponsiveFullScreen #6898

Conversation

rosskevin commented May 18, 2017 • edited

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented May 18, 2017 • edited

oliviertassinari commented May 18, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari May 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskevin commented May 19, 2017

Types of Dialogs:

Responsive

Back to types

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented May 19, 2017

rosskevin commented May 19, 2017

rosskevin commented May 19, 2017

oliviertassinari May 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari May 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Dialog] create responsive HOC `withResponsiveFullScreen` #6898

[Dialog] create responsive HOC `withResponsiveFullScreen` #6898

rosskevin commented May 18, 2017 •

edited

oliviertassinari left a comment •

edited

oliviertassinari commented May 18, 2017 •

edited

oliviertassinari May 19, 2017 •

edited

oliviertassinari May 20, 2017 •

edited

oliviertassinari May 20, 2017 •

edited