Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation to use reference object as replacement to DOM node #193

Closed
maxkfranz opened this issue Mar 28, 2017 · 27 comments
Closed
Labels
docs No code, just documentation. feature This would be nice to have. PRIORITY: low TARGETS: core Utility functions, lifecycle, core library stuff.

Comments

@maxkfranz
Copy link

It would be nice if a function could be passed instead of an html element for reference at init. The function would return a bounding box that could be used for positioning.

The function is necessary --- rather than just a plain object --- to allow for cases where the bounding box would change over time. This would allow updates with popper.update().

Having this feature would make support for libs like Cytoscape much better.

Thanks for your consideration.

@FezVrasta
Copy link
Member

That's something I'd like to add but I haven't time at the moment. There's some work needed to make this idea work.

@FezVrasta
Copy link
Member

FezVrasta commented Apr 2, 2017

More context on the feature's requirements.

It should provide all the informations needed by Popper.js to be able to compute the position of the popper, for instance:

  • Context of the fake element (where "is" it located in the DOM?) to make it possible to properly position the popper;
  • All its sizes (everything outputted by getBoundingClientRect())
  • Popper.js must be changed to properly handle a function instead of an element. Specifically these functions:
    • getReferenceOffsets
    • getPosition

There are the major points I can think about.

An alternative approach would be to create a custom modifier that hooks into the onLoad lifecycle and creates the reference node starting from the data you provide and sets it as reference element. This approach is easier even if less efficient because it needs to append a new DOM node.

@FezVrasta FezVrasta changed the title Feature request : function as reference Allow to use function as reference Apr 2, 2017
@maxkfranz
Copy link
Author

@FezVrasta Would it be possible to pass in a DOM-like object (or have the function return such an object)? For example:

let makeFakeDiv () => {
  return {
    getBoundingClientRect: function(){ /* ... */ },
    parentElement: document.body
  };
};

let popper1 = new Popper( makeFakeDiv() );

// or 

let popper2 = new Popper( makeFakeDiv );

This would probably not require as invasive changes, but the list of fields expected for the fake DOM object would have to be documented. Even popper1 can still be dynamic by changing what getBoundingClientRect() returns, so it would suffice for the Cytoscape.js usecase.

@FezVrasta
Copy link
Member

As long your custom object "looks" like a DOM node I think Popper.js shouldn't complain about it.

@bedeoverend
Copy link

+1 for this. IMO the "DOM-like" object would be best. After some digging, it looks like the following interface would be needed:

getBoundingClientRect() : DOMRect
clientWidth : Integer
clientHeight : Integer
offsetParent : HTMLElement? = window.document.documentElement

Is there anything I'm missing? Obviously DOMRect you'd just need the interface, as constructing a literal DOMRect isn't supported in most browsers and isn't needed I assume, just so long as it has the same properties.

@FezVrasta
Copy link
Member

I guess something needs to change in Popper.js to tell it to call the "DOM-like-object-factory-function-wahtever" and update clientWidth, clientHeight and offsetParent if you don't want to use getters

@bedeoverend
Copy link

I'd be pro getters and no factory function - it would mean this would be a smaller PR, without closing the door on a function in future?

But if not, what about an onBeforeUpdate callback? People could hook into that and update their "DOM-like" object's props? Though I guess that essentially comes to the same thing as getters anyway.

@FezVrasta
Copy link
Member

Do you need a PR if you use getters? 🤔

I'm not against them, I'm not updated about the browser support and I think that a change like this would be quite minimal:

if(typeof reference === 'function') reference = reference() // I mean, with an actual type checking that works, lol

@FezVrasta
Copy link
Member

Ok, updated my informations:
image

img

@bedeoverend
Copy link

Hahaha - well down to IE9 is enough for popper yeah?

Ah yeah, can start using it straight away 😉 I guess just documenting that it reference needs to match this interface, rather than an HTMLElement?

Well, I know my use case would be covered purely by getters, but @maxkfranz does it cover your needs?

@FezVrasta
Copy link
Member

Please do let me know if it works for you and share some working code if you can.

@bedeoverend
Copy link

Will do - I'll try get something together in the next few days

@FezVrasta
Copy link
Member

FezVrasta commented Apr 5, 2017

I put together this:
http://codepen.io/FezVrasta/pen/evoMrR?editors=1010

I'm not sure if everything works, we should add a set of tests to make sure everything works properly.

Probably Popper.js could expose a method to generate this object:

const ref = Popper.genReference({ top, left, right, bottom });
new Popper(ref, pop);

@bedeoverend
Copy link

+1 for tests. I created a fork of that codepen and it looks to be playing pretty nicely with a range - which is my use case, essentially having a toolbar object hover over text selection.

@FezVrasta
Copy link
Member

Damn that's really cool! Great work

@FezVrasta FezVrasta added feature This would be nice to have. and removed IDEA labels Apr 6, 2017
@FezVrasta
Copy link
Member

I referenced #9 because seems like a natural step to support @bedeoverend use case.

I'm not sure how to proceed with it, basically we need to support a "non rectangular" reference element/area...

@FezVrasta FezVrasta changed the title Allow to use function as reference Implement reference object generator as replacement to DOM node Apr 6, 2017
@Justineo
Copy link

Justineo commented Apr 6, 2017

@FezVrasta I tried to implement #9 earlier but when I dug into the code a little bit I feel uncertain about how should that fit into current calculations.

@FezVrasta
Copy link
Member

@Justineo actually I have no idea as well on how to proceed 😶

@maxkfranz
Copy link
Author

As long as some kind of dom-like object is officially supported/documented as reference, that works for me.

@FezVrasta
Copy link
Member

FezVrasta commented Apr 6, 2017

@maxkfranz then you can start using something on the line of the codepen I posted here.

I'll add the generator we talked about once I have some free time, but it's not needed to start using this method.

@maxkfranz
Copy link
Author

@FezVrasta Perfect, thanks. I agree that the generator is just a nice-to-have.

@FezVrasta
Copy link
Member

Please @maxkfranz and @bedeoverend, may you review this PR and let me know if everything looks good to you?

#197

@bedeoverend
Copy link

Awesome! Thanks @FezVrasta.

Personally, I'd prefer if it were documented that reference just had to match a certain interface i.e. be an object which has getBoundingClientRect() function and clientWidth / clientHeight properties (and maybe offsetParent). That way I'm not limited to just using HTMLElement or the new Reference class. But I'm not sure if that's shared by anyone or just me. I also understand that that could be annoying as it means you have to have backwards compatibility for it going forward.

@FezVrasta
Copy link
Member

actually I'd like to avoid to add more code to the library since it's growing too much for my tastes in the past months.. Maybe I could simply document as you suggested and put the sample ReferenceClass in the docs folder (or a gist link) for reference?

@bedeoverend
Copy link

Yeah I think that sounds good. A gist or a guide on how to use it would be good 👍

@maxkfranz
Copy link
Author

I agree it's better / more flexible to just have documentation.

@FezVrasta FezVrasta changed the title Implement reference object generator as replacement to DOM node Add documentation to use reference object as replacement to DOM node Apr 13, 2017
@FezVrasta FezVrasta added the docs No code, just documentation. label Apr 13, 2017
@FezVrasta
Copy link
Member

Looks like the recent changes introduced in 1.5.1 broken the possibility to use an object as reference element.

We should update the code to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs No code, just documentation. feature This would be nice to have. PRIORITY: low TARGETS: core Utility functions, lifecycle, core library stuff.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants