feat: add support for virtual elements in popper and popover

[HendrikThePendric]

This feature is needed to unblock some work that @edoardo is doing. I've included some stories that are demoing the functionality, but no e2e tests. I'd like to pick that up later, once this work has gone into @dhis2/ui.

[ismay]

So if I understand correctly, the usecase here is to allow placement of a popper without a reference element? Where the virtual element is basically supplying position information, without there actually being an element?

[HendrikThePendric]

Yeah, it's nice to support this anyway for example if you want a popper to follow a mouse cursor or something.

The specific issue @edoardo encountered was as follows:

• When clicking in a table-cell, a popper with a menu should show, positioned against that table cell.
• However, on click the table re-renders, so the ref becomes null
• But before that re-render, we would still be able to access the DOM node and call .getBoundingClientRect() on it.
• This way we'd still be able to pass the right position to the Popper.

I guess this isn't something that will happen a lot, but nice to have support for it anyway.

I also quite like the fact that our Popper now has support for all types of references that are relevant:

• The objects created by useRef or createRef
• Plain DOM nodes
• Virtual elements (which is supported in popper.js)

[ismay]

Yeah, it's nice to support this anyway for example if you want a popper to follow a mouse cursor or something.

The specific issue @edoardo encountered was as follows:

* When clicking in a table-cell, a popper with a menu should show, positioned against that table cell.

* However, on click the table re-renders, so the ref becomes null

* But before that re-render, we would still be able to access the DOM node and call `.getBoundingClientRect()` on it.

* This way we'd still be able to pass the right position to the Popper.

I guess this isn't something that will happen a lot, but nice to have support for it anyway.

Hmm yeah, it sounds like we need better support for attaching a popper in this case, using a virtual element seems like it could still go wrong if the table decides to render differently for some reason and then the measurement would be wrong. Would there not be a more robust solution, something that utilizes the actual ref somehow?

[HendrikThePendric]

Hmm yeah, it sounds like we need better support for attaching a popper in this case, using a virtual element seems like it could still go wrong if the table decides to render differently for some reason and then the measurement would be wrong. Would there not be a more robust solution, something that utilizes the actual ref somehow?

I think I disagree with that, at least from the position of the ui-core library... The popper needs some sort of reference point to position against. It's up to the app to provide one. Passing a "virtual element" proved to be a good solution in this case, and for the popper to accept these as reference makes sense because the positioning engine supports that too. However, if the situation you were describing would occur, the table re-rendering into a different layout, then it's up to the app to determine the position of the popper.

[ismay]

The code changes themselves look good to me. I do wonder about the usecase it's solving though. If this is a common thing we should have a more robust solution I think, unless that's just not possible (I don't know). I would also prefer to add some tests before merging, personally. It's not a huge change and I understand that there's time pressure, so I'll leave the decision to @HendrikThePendric as you know more than me about what @edoardo is working on and the deadlines involved.

[ismay]

I think I disagree with that, at least from the position of the ui-core library... The popper needs some sort of reference point to position against. It's up to the app to provide one. Passing a "virtual element" proved to be a good solution in this case, and for the popper to accept these as reference makes sense because the positioning engine supports that too. However, if the situation you were describing would occur, the table re-rendering into a different layout, then it's up to the app to determine the position of the popper.

Yeah the thing is, I can see this being used as a shortcut for these kinds of problems. And it seems like a shortcut that could break. If this is a problem that people are encountering I think we should investigate if it's possible to solve this in a robust way. Otherwise I can imagine this pattern would proliferate and that might not be a good thing.

[HendrikThePendric]

so I'll leave the decision to @HendrikThePendric as you know more than me about what @edoardo is working on and the deadlines involved.

It's still being discussed in the analytics-team, but it could well be that these two ui-core fixes (the current one and #888) will enable some of @edoardo work to make it into the 2.34.1 release, provided this is released today (or I guess first thing tomorrow).

I'd also like to add tests for this work, but would probably do so after we've pulled all of this into @dhis2/ui.

[cypress[bot]]

---

Test summary

335 • 0 • 0 • 0

---

Run details

Project	ui-core
Status	Passed
Commit	ee75171e9c
Started	May 19, 2020 3:20 PM
Ended	May 19, 2020 3:28 PM
Duration	07:44 💡
OS	Linux Ubuntu Linux - 18.04
Browser	Electron 78

View run in Cypress Dashboard ➡️

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[dhis2-bot]

:tada: This PR is included in version 4.21.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: