search

thednp / bootstrap.native

Bootstrap components build with Typescript
http://thednp.github.io/bootstrap.native/
MIT License
1.7k stars 177 forks source link

Popover customisation and issues #377

Closed sookoll closed 4 years ago

sookoll commented 4 years ago

I have several issues with popovers converting from jquery to native.

With jquery and Popper, I did:

Popper.Defaults.modifiers.preventOverflow.enabled = false
Popper.Defaults.modifiers.hide.enabled = false

and

// el is html node
$(el).popover({
  container: el,
  placement: 'right',
  offset: (this.state.items.length - 1) * 20 + 'px, 0'
})

to calculate correct position on openlayers map (based on example here: https://openlayers.org/en/latest/examples/overlay.html)

Works like a charm: ezgif com-video-to-gif

Now, with BSN:

const popover = new Popover(el, {
  container: el,
  placement: 'right',
  trigger: 'click',// can't use 'manual'
})

Basically it works, at least it renders. But:

  1. For some reason (probably related with openlayers overlay positioning), popover are placed as much right/down from mouse click position as page left/top edge and mouse click position. It seems, that it do not calculate left/top from container but from page.
  2. Moving map will leave some ghosting on screen.
  3. There is no offset option, how can I set pointer position relative to content?
  4. If I open popover near right side of screen, it automatically change placement from right to left. How to prevent that?

None of these issues happen with jquery plugin.

This happens: ezgif com-video-to-gif-3

sookoll commented 4 years ago

  1. temporary fix (more like hack) for me is:

    top: 0 !important
left: 0 !important

    for element css. But it is not general solution and not suite other cases, where container is in use.

  2. ghosting was originated from element css filter: drop-shadow effect. Without it no screen ghosting happen. It is weird, as it do not happen with jquery popover plugin.

thednp commented 4 years ago

You can fork the library and add this somewhere in the toggleEvents your own 'manual' implementation. Make sure to do popover-custom.js, add this into your bundle instead of popover-native.js.

I don't know/think this to be critical for everyone though.

sookoll commented 4 years ago

Wait... what!? If You use container, but still calculate popover position from page, then it is a bug. You think, that offset parameter is pointless possibility in original plugin? This library should give users as close usage and result to original bootstrap as possible. Otherwise, what's the point of this? Or You consider this library as mature, and do only critical bugfixes? On the other hands, Bootstrap v5 will drop jquery dependency, so yeah, why bother...

thednp commented 4 years ago

The offsets for Tooltip and Popover as calculated around the container. Or I don't understand your issue?

thednp commented 4 years ago

What I mean is: we don't use Popper, we only implemented something similar to better calculate offsets, without the entire Popper. But if you can get around the difficulty with a simple CSS, I don't see the problem.

For your need, to have it exactly your way, you might have to fork, make it exactly that way, or wait for BS V5.

sookoll commented 4 years ago

Offsets are for let user to customize how popover are positioned relative to target. Let say, I want that popover are higher above button than default. Or like this Github comment block, where trigger is user avatar and tip pointing that is on left-top corner. This is achieved with offsets.

So You do not fix a bug and do not consider offset parameter to let users to customise popover behaviour. So hurry to close issues, without waiting if anybody else have any opinion on this. Yeah, of course, I can fork and improve it myself, drop it for some other replacement, wait BS v5 and so on...

thednp commented 4 years ago

This is not a bug @sookoll, and I'm not trying to be disrespectful, but this is a feature that most of us don't need. Instead of going full on bloating the library, we prefer to have it free of dependencies, light, fast, reliable and flexible. If you check the docs (on the demo), there is no offset option advertised for Popover.

The easiest way to "fix" bugs is to just report them here, the easiest way to implement a new feature is to open an issue, have a chat with the community, and implement if it's really important. So if 1 in 2000 users need that feature, it's not implemented. Makes sense?

sookoll commented 4 years ago

Please read again. Offsets are not reported as bug, Popover placement calculation on container is a bug. Jquery plugin does it correctly, this doesn't. Agree, that offsets option is new feature, but Bootstrap have it, so this library should have it also. But of course, You can decide whatever You like.

thednp commented 4 years ago

Yea, I don't think this feature is useful. In my mind the easiest and bestest way I would do it anyway:

Easy JavaScript! Good day.