[Popover] Refactor popover transition - separation of concerns #7720

rosskevin · 2017-08-09T23:30:01Z

squash+merge

For simplification and external reuse of the Popover transition (now called Grow) with react-autosuggest, I have refactored Popover. Popover is currently used exclusively by Menu.

As it stands, here are the purposes:

internal/Popover = positioning + Modal + Grow + Paper (used by Menu)
transitions/Grow = only the effect exhibited by popovers - grow with a bit of fade in.

There are also various small fixes I encountered as I refactored.

I should also mention that this is possibly the lead-in to deleting the positioning code in Popover in lieu of using react-popper, but I'll wait to take that on after I've used it a bit with react-autosuggest.

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).

…other

rosskevin · 2017-08-09T23:31:08Z

NOTE: I have seen cross-contamination of tests when run sequentially. I could use some help with that.

Individually, tests are passing and the Grow.spec.js in fact never uses the value woof, so I suspect that the virtual DOM has something left over or there is some sort of race condition from a previous test.

I have changed all references to woof to isolate the cross-contamination:

  1101 passing (3s)
  3 failing

  1) <Grow /> event callbacks should fire event callbacks:
     Error: Warning: Material-UI: argument "duration" must be a number but found woofCollapse
      at Console.console.error (test/utils/consoleError.js:8:5)
      at warning (node_modules/warning/warning.js:51:17)
      at Object.create (src/styles/transitions.js:66:5)
      at Grow._this.handleEnter (src/transitions/Grow.js:92:19)
      at node_modules/enzyme/build/ShallowWrapper.js:844:23
      at ReactDefaultBatchingStrategyTransaction.perform (node_modules/react-test-renderer/lib/shallow/Transaction.js:143:20)
      at Object.batchedUpdates (node_modules/react-test-renderer/lib/shallow/ReactDefaultBatchingStrategy.js:62:26)
      at Object.batchedUpdates (node_modules/react-test-renderer/lib/shallow/ReactUpdates.js:97:27)
      at ReactShallowRenderer.unstable_batchedUpdates (node_modules/react-test-renderer/lib/shallow/ReactShallowRenderer.js:130:25)
      at performBatchedUpdates (node_modules/enzyme/build/ShallowWrapper.js:103:21)
      at node_modules/enzyme/build/ShallowWrapper.js:843:13
      at withSetStateAllowed (node_modules/enzyme/build/Utils.js:284:3)
      at ShallowWrapper.simulate (node_modules/enzyme/build/ShallowWrapper.js:840:42)
      at src/transitions/Grow.spec.js:29:17
      at Array.forEach (native)
      at Context.<anonymous> (src/transitions/Grow.spec.js:27:14)

  2) <Grow /> transition lifecycle handleEnter(element) "before all" hook:
     Error: Warning: Material-UI: argument "duration" must be a number but found woofCollapse
      at Console.console.error (test/utils/consoleError.js:8:5)
      at warning (node_modules/warning/warning.js:51:17)
      at Object.create (src/styles/transitions.js:66:5)
      at Grow._this.handleEnter (src/transitions/Grow.js:92:19)
      at Context.<anonymous> (src/transitions/Grow.spec.js:55:28)

  3) <Grow /> transition lifecycle handleExit(element) "before all" hook:
     Error: Warning: Material-UI: argument "duration" must be a number but found woofCollapse
      at Console.console.error (test/utils/consoleError.js:8:5)
      at warning (node_modules/warning/warning.js:51:17)
      at Object.create (src/styles/transitions.js:66:5)
      at Grow._this.handleExit (src/transitions/Grow.js:120:19)
      at Context.<anonymous> (src/transitions/Grow.spec.js:104:28)

…ross contamination

rosskevin · 2017-08-10T00:50:00Z

This appears to be stubbed and perhaps never unstubbed:

theme.transitions.getAutoHeightDuration = stub().returns('woofCollapse');

rosskevin · 2017-08-10T00:55:15Z

@oliviertassinari this recently changed from a styleManagerMock to an assignment on theme.
Was:

         styleManagerMock = wrapper.context('styleManager');
          styleManagerMock.theme.transitions.getAutoHeightDuration = stub().returns('woof');
          wrapper.setContext({ styleManager: styleManagerMock });
          wrapper.setProps({ transitionDuration: 'auto' });
          instance = wrapper.instance();

Now:

          theme = instance.props.theme;
          theme.transitions.getAutoHeightDuration = stub().returns('woofCollapseStub');
          wrapper.setProps({ transitionDuration: 'auto' });
          instance = wrapper.instance();

Either we need to regenerate theme so it is pristine on each shallow | mount, or this test needs to go back to using a mock. What are your thoughts?

rosskevin · 2017-08-10T02:07:20Z

I added an explicit restore of the stubbed method.

oliviertassinari

I like the idea of this PR. It's a quick review, I need to have a deeper look this evening :).

oliviertassinari · 2017-08-10T09:15:27Z

src/internal/Popover.js

@@ -200,7 +200,7 @@ type AllProps = DefaultProps & Props;
 /**
 * @ignore - internal component.
 */
-class Popover extends Component<DefaultProps, AllProps, void> {
+class Popover extends PureComponent<DefaultProps, AllProps, void> {


The component takes a children, let's keep Component.

oliviertassinari · 2017-08-10T09:16:30Z

src/transitions/Grow.js

+/**
+ * @ignore - internal component.
+ */
+class Grow extends PureComponent<DefaultProps, AllProps, void> {


The component takes a children, PureComponent is harming the performance, let's use to Component.

rosskevin · 2017-08-10T14:38:57Z

I've confirmed the Grow to work as expected when used externally.

FezVrasta · 2017-08-10T15:11:20Z

Feel free to ping me if you need help or more info about react-popper and Popper.js in general.

rosskevin · 2017-08-10T15:14:39Z

Thanks @FezVrasta - hope you are well.

oliviertassinari

I had a deep look into it. That looks great 😄 .

oliviertassinari · 2017-08-10T17:49:51Z

src/internal/Popover.js

@@ -479,7 +411,7 @@ class Popover extends Component<DefaultProps, AllProps, void> {
            <EventListener target="window" onResize={this.handleResize} />
            {children}
          </Paper>
-        </Transition>
+        </Grow>
      </Modal>
    );
  }


You can remove all the occurrences of withTheme and theme in this file.

still need it in #handleEnter for theme.transitions

I can't find that.

Sorry, I was looking at the wrong file, you are right.

oliviertassinari · 2017-08-10T17:50:42Z

src/internal/Popover.js

@@ -449,24 +381,24 @@ class Popover extends Component<DefaultProps, AllProps, void> {
      ...other
    } = this.props;

+    // FIXME: props API consistency problem? - `...other` not spread over the root


I agree, let's move the ...other to the root. What's preventing us from doing it?

Nothing yet, just want to save that for the next wave of the refactor.

oliviertassinari · 2017-08-10T17:51:53Z

src/transitions/Grow.js

+}
+
+type DefaultProps = {
+  theme: Object,


This property is injected by withTheme(), I don't think that we need it.

necessary for flow resolution. We could add it to AllProps instead, but that would differ from the pattern in other files.

Won't making the property required allow us to remove it here? It's the same for the classes.

It has to be required somewhere, and it is not required outside this component. If it is not required outside the component, we cannot require it in export type Props, as it would prohibit using the type externally to compose components.

oliviertassinari · 2017-08-10T17:52:10Z

src/transitions/Grow.js

+  /**
+   * @ignore
+   */
+  theme?: Object,


I think that it should be required.

It's not required externally (maybe type), but it is internally, that's why it's optional in Props and required in DefaultProps. The intersection of these mean internally the component can expect it to be non-null.

Isn't the same thing with the classes? I have put them required in the code, but in the documentation I have change them to be optional. Anyway here it's not documented.

Same as classes, see above for explanation. It's all about externalizing Props for reuse. If theme isn't allowed to be passed in (by the user), we should remove it from Props.

oliviertassinari · 2017-08-10T17:54:17Z

src/transitions/Grow.js

+import Transition from '../internal/Transition';
+import type { TransitionCallback } from '../internal/Transition';
+
+export function getScale(value: number) {


Maybe we should add // Only exported for the tests.

oliviertassinari · 2017-08-10T18:04:19Z

I should also mention that this is possibly the lead-in to deleting the positioning code in Popover in lieu of using react-popper

I'm not that sure about it. The Popover has a getContentAnchorEl property to handle a specific positioning requirement of the material specification.

rosskevin · 2017-08-10T18:15:03Z

I'm not that sure about it. The Popover has a getContentAnchorEl property to handle a specific positioning requirement of the material specification.

This can be done with popper, even the custom positioning with a modifier. I'm not too keen to take that on yet. I've got a lot on my plate, but it is certainly viable and I think it would cut code and rely on a good external project.

oliviertassinari · 2017-08-10T19:32:18Z

@rosskevin Great, it's much better with the concerns separated :).

* upstream/v1-beta: (24 commits) v1.0.0-beta.5 Update CHANGELOG.md [CHANGELOG] Prepare v1.0.0-beta.5 chore(package): update size-limit to version 0.9.0 (mui#7757) [docs] Fix yarn docs:build script (mui#7745) [docs] Update the readme [Radio] Fix accessibility issue (mui#7742) [Tabs][BottomNavigation] Use value over index property (mui#7741) [core] Remove createStyleSheet (mui#7740) [core] Start simplifying styling API (mui#7730) [LinearProgress] Use transform instead width (mui#7732) Fix Typo (mui#7736) Documentation Fix - Fixing up the README documentation (mui#7733) [ButtonBase] Expose internal component (mui#7727) [Popover] Refactor popover transition - separation of concerns (mui#7720) Button documentation fix (mui#7726) Update supported-components.md (mui#7722) chore(package): update webpack to version 3.5.3 (mui#7723) [flow] flow type transitions Slide, Fade, Collapse (fixes) Create CODE_OF_CONDUCT.md ...

rosskevin added 8 commits August 9, 2017 16:39

[Popover] initial extraction of transition only code

980c810

[flow] fixes for Slide and Popover

1d10b04

[Menu] cleanup - minimize explicit passing of props and fallback to ……

f7ffa38

…other

[Popover] refactor internal/Popover to use transitions/Popover

5d9daaf

[flow] strengthen flow typing on transitions.js

1a5dcaa

[Popover] separate specs between positioning and transition

ca0c4e3

[transitions] make warning more informative

bf63c9a

Rename transitions/Popover to Grow

0ac0cfa

rosskevin added core labels Aug 9, 2017

rosskevin requested a review from oliviertassinari August 9, 2017 23:30

rosskevin added 2 commits August 9, 2017 19:34

Replace generic test value woof with woof<filename> to identify c…

6532871

…ross contamination

lint

829afb4

identify cross contamination inside Collapse.spec

cb3c51a

explicit restore of stubbed method

78ca884

oliviertassinari reviewed Aug 10, 2017

View reviewed changes

rosskevin added 2 commits August 10, 2017 08:29

Switch back to Component instead of PureComponent

e399aa9

[docs] build

b9b785b

rosskevin mentioned this pull request Aug 10, 2017

[Autocomplete] Show an integration example #7477

Merged

3 tasks

Update Collapse.spec.js

7a954bc

oliviertassinari reviewed Aug 10, 2017

View reviewed changes

rosskevin added 2 commits August 10, 2017 13:15

add comment

ee6bc4d

[Popover] theme is unused

674f6ce

oliviertassinari approved these changes Aug 10, 2017

View reviewed changes

oliviertassinari merged commit 640e56e into mui:v1-beta Aug 10, 2017

rosskevin deleted the refactor-popover-transition branch August 16, 2017 21:05

zannager added the component: Popover The React component. label Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Popover] Refactor popover transition - separation of concerns #7720

[Popover] Refactor popover transition - separation of concerns #7720

rosskevin commented Aug 9, 2017 •

edited

rosskevin commented Aug 9, 2017 •

edited

rosskevin commented Aug 10, 2017

rosskevin commented Aug 10, 2017 •

edited

rosskevin commented Aug 10, 2017

oliviertassinari left a comment

oliviertassinari Aug 10, 2017

oliviertassinari Aug 10, 2017

rosskevin commented Aug 10, 2017

FezVrasta commented Aug 10, 2017

rosskevin commented Aug 10, 2017

oliviertassinari left a comment

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017 •

edited

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017

oliviertassinari Aug 10, 2017

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017 •

edited

oliviertassinari Aug 10, 2017

rosskevin Aug 10, 2017 •

edited

oliviertassinari Aug 10, 2017

oliviertassinari commented Aug 10, 2017

rosskevin commented Aug 10, 2017

oliviertassinari commented Aug 10, 2017

[Popover] Refactor popover transition - separation of concerns #7720

[Popover] Refactor popover transition - separation of concerns #7720

Conversation

rosskevin commented Aug 9, 2017 • edited

rosskevin commented Aug 9, 2017 • edited

rosskevin commented Aug 10, 2017

rosskevin commented Aug 10, 2017 • edited

rosskevin commented Aug 10, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskevin commented Aug 10, 2017

FezVrasta commented Aug 10, 2017

rosskevin commented Aug 10, 2017

oliviertassinari left a 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 Aug 10, 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

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 Aug 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskevin Aug 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Aug 10, 2017

rosskevin commented Aug 10, 2017

oliviertassinari commented Aug 10, 2017

rosskevin commented Aug 9, 2017 •

edited

rosskevin commented Aug 9, 2017 •

edited

rosskevin commented Aug 10, 2017 •

edited

rosskevin Aug 10, 2017 •

edited

rosskevin Aug 10, 2017 •

edited

rosskevin Aug 10, 2017 •

edited