[Popover] Fixed bug where Popover renders relative to screen #7663

lostpebble · 2017-08-04T09:34:52Z

A continuation of issue #7605

Something has changed in React Fiber which might have something to do with unstable_renderSubtreeIntoContainer which PopOver (and RenderToLayer inside of it) uses to render itself inside a container which initiated the click / opening event.

It might also have something to do with mounting order, as PopOver tries to render itself inside of one of its child elements. To ensure that the child layer is created correctly before attempting to render, a timeout is set at 1ms inside PopOver's componentDidMount before rendering the PopOver inside the layer. It's a quick fix, but at least it works.

From what I can gather in my projects, this should be the last step in making pre-V1 compatible with React Fiber.

In this pull request as well: Changed the tests for [Menu] to be in line with the previous pull request #7624

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

…created correctly, after [RenderToLayer] has completed it's render

…r layout to be setup correctly). Also fixed [Menu] tests to work with recent onTouchTap -> onClick changes.

oliviertassinari · 2017-08-05T06:19:20Z

src/Popover/Popover.js

    this.state = {
      open: props.open,
      closing: false,
    };
  }

  componentDidMount() {
-    this.setPlacement();
+    setTimeout(this.setPlacement, 1);


That's hidden something, I would normally ask to look into it, we shouldn't have such code. But anyway, the master is no longer a priority. Also, that might lead to other potential edge cases.

Can we just clear the timeout when unmounting? Also, no need to set 1, 0 or no argument is enough.

Okay cool, I'll make those changes and do the clear on unmount. Just figured it would be over so quick that clearing it is irrelevant - but I guess these things all work on a much more micro scale.

oliviertassinari · 2017-08-05T06:22:06Z

Something has changed in React Fiber

I confirm the behavior, I have noticed it to some time ago in the v1-beta branch. We ended-up using the new portal API.

oliviertassinari · 2017-08-05T06:22:59Z

I'm not suggesting we do such, the fewer changes, the less likely we introduce a regression, the less time we have to spend on it, the better :).

lostpebble · 2017-08-05T07:59:22Z

Yep not ideal code, but yea I'm just hoping to get the pre-v1 branch compatible before V1 catches up with some of the components. Even if it's slightly dirty.

…elay

oliviertassinari · 2017-08-05T09:38:54Z

@lostpebble Thanks

lostpebble · 2017-08-05T11:15:48Z

@oliviertassinari Happy to help out :)

Thanks for all your work on this project. Looking forward to V1! I'll see if I can find time to contribute a bit to that branch as well to get those components I need up to speed.

oliviertassinari · 2017-08-05T11:18:47Z

@lostpebble Which components do you miss from the v1-beta branch? Could you 👍 on the issues you want to see first?

lostpebble · 2017-08-05T11:28:05Z

Sure, will do that. It's pretty much just DatePicker and SelectField I think, and probably Drawer but might be creating my own implementation of that for my project.

oliviertassinari · 2017-08-05T11:30:43Z

@lostpebble We will release v1 before migrating the DatePicker, we miss some documentation for the Drawer and to finish #6878. The SelectField is by far the most requested component. I will try to work on it.

* call_em_all_-_master/master: [showcase] added showcase of 'HoopHubs.com' (mui#7677) [PopOver] Fixed bug where PopOver renders relative to screen rather than layout container in React Fiber (mui#7663)

…rather than layout container in React Fiber (mui#7663)" This reverts commit 8432807.

lostpebble added 7 commits August 1, 2017 18:12

replaced all occurances of onTouchTap with onClick

4dfa533

changed tests to use 'click' event, and seem to all be passing.

2e34d9e

[eslint] Alphabetically ordered onClick in props for code quality

90d92ee

[Popover] Added 1ms timeout to render popover to ensure placement is …

ca7f833

…created correctly, after [RenderToLayer] has completed it's render

resolved merge conflict

eeb0486

[Popover] removed unused development prop on [RenderToLayer]

c1a1091

[PopOver] Fixed tests to be inline with the new behaviour (waiting fo…

6c3814c

…r layout to be setup correctly). Also fixed [Menu] tests to work with recent onTouchTap -> onClick changes.

oliviertassinari reviewed Aug 5, 2017

View reviewed changes

[PopOver] cleared timeout on unmount and simplified timeout without d…

b4fffa8

…elay

oliviertassinari approved these changes Aug 5, 2017

View reviewed changes

oliviertassinari added the PR: Review Accepted label Aug 5, 2017

oliviertassinari merged commit 8432807 into mui:master Aug 5, 2017

lostpebble deleted the fix-popover-rv16 branch August 5, 2017 10:10

oliviertassinari changed the title ~~[PopOver] Fixed bug where PopOver renders relative to screen rather than layout container in React Fiber~~ [Popover] Fixed bug where Popover renders relative to screen rather than layout container in React Fiber Aug 15, 2017

oliviertassinari added the component: Popover The React component. label Aug 15, 2017

oliviertassinari changed the title ~~[Popover] Fixed bug where Popover renders relative to screen rather than layout container in React Fiber~~ [Popover] Fixed bug where Popover renders relative to screen Aug 15, 2017

therahl mentioned this pull request Sep 4, 2017

[Popover] Positioning delayed #8040

Closed

igorokb added a commit to igorokb/material-ui that referenced this pull request Sep 15, 2017

Revert "[PopOver] Fixed bug where PopOver renders relative to screen …

5110d4a

…rather than layout container in React Fiber (mui#7663)" This reverts commit 8432807.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Popover] Fixed bug where Popover renders relative to screen #7663

[Popover] Fixed bug where Popover renders relative to screen #7663

lostpebble commented Aug 4, 2017

oliviertassinari Aug 5, 2017 •

edited

Loading

lostpebble Aug 5, 2017 •

edited

Loading

oliviertassinari commented Aug 5, 2017 •

edited

Loading

oliviertassinari commented Aug 5, 2017 •

edited

Loading

lostpebble commented Aug 5, 2017

oliviertassinari commented Aug 5, 2017

lostpebble commented Aug 5, 2017 •

edited

Loading

oliviertassinari commented Aug 5, 2017 •

edited

Loading

lostpebble commented Aug 5, 2017

oliviertassinari commented Aug 5, 2017

[Popover] Fixed bug where Popover renders relative to screen #7663

[Popover] Fixed bug where Popover renders relative to screen #7663

Conversation

lostpebble commented Aug 4, 2017

oliviertassinari Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

lostpebble Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Aug 5, 2017 • edited Loading

oliviertassinari commented Aug 5, 2017 • edited Loading

lostpebble commented Aug 5, 2017

oliviertassinari commented Aug 5, 2017

lostpebble commented Aug 5, 2017 • edited Loading

oliviertassinari commented Aug 5, 2017 • edited Loading

lostpebble commented Aug 5, 2017

oliviertassinari commented Aug 5, 2017

oliviertassinari Aug 5, 2017 •

edited

Loading

lostpebble Aug 5, 2017 •

edited

Loading

oliviertassinari commented Aug 5, 2017 •

edited

Loading

oliviertassinari commented Aug 5, 2017 •

edited

Loading

lostpebble commented Aug 5, 2017 •

edited

Loading

oliviertassinari commented Aug 5, 2017 •

edited

Loading