popper.js.org icon indicating copy to clipboard operation
popper.js.org copied to clipboard

WIP: v3 docs

Open atomiks opened this issue 4 years ago • 8 comments

atomiks avatar Nov 22 '21 12:11 atomiks

Forgot to add the auto modifier in... I believe we shouldn't add auto as a Placement. This means we can remove all the checks around that which simplifies types. Instead, auto should just take it as an option.

auto() // 'auto' default
auto('start') 'auto-start'
auto('end') 'auto-end'

atomiks avatar Nov 22 '21 14:11 atomiks

Bunch of other ideas floating around in my head:

1.

Instead of a modifier, placement could accept a tree-shakeable piece of code.

import { position, auto } from '@popperjs/dom';

position(reference, popper, {
  placement: auto()
});

2.

We should probably call the main API computePosition, since it's pure, it's more descriptive... We could also just call it Popper:

import { Popper } from '@popperjs/dom';

Popper(reference, popper).then(...);

3.

shift() could take limit as a tree-shakeable function, maybe the property could be called limiter. This couples the code better and avoids ordering issues. It's a bit nonconventional though.

import { computePosition, shift, shiftLimiter } from '@popperjs/dom'; 

computePosition(reference, popper, {
  modifiers: [
    shift({ limiter: shiftLimiter }),
  ]
});

atomiks avatar Nov 22 '21 15:11 atomiks

  1. The problem with that is that users of libraries such as Bootstrap which don't have usually access to JS will not be able to use the auto placement. I'm wondering if we should also expose a wrapper around the new API that exposes something more in line with the v2 interface to ease the adaoption.
  2. I don't have a strong preference but let's not use capital names since they are usually reserved for classes
  3. I like it

FezVrasta avatar Nov 22 '21 16:11 FezVrasta

  1. It doesn't look like Bootstrap 5 offers the auto placement at all 🤔 https://getbootstrap.com/docs/5.0/components/tooltips/. imo, we shouldn't provide any wrappers, because Bootstrap can create their own mini-wrapper. The whole point of the API is users can do whatever they want to suit their needs.
  2. I agree, problem is popper is often used lowercase to refer to the element 😒 computePosition is likely the best bet for now, even if it's longer

atomiks avatar Nov 22 '21 16:11 atomiks

  1. It doesn't look like Bootstrap 5 offers the auto placement at all 🤔 getbootstrap.com/docs/5.0/components/tooltips. imo, we shouldn't provide any wrappers, because Bootstrap can create their own mini-wrapper. The whole point of the API is users can do whatever they want to suit their needs.

@XhmikosR would love to hear your input on this

FezVrasta avatar Nov 22 '21 16:11 FezVrasta

Also, I think it would be best if authors prevented user access to Popper's details. Bootstrap 5 can't upgrade to v3, because they provide access to Popper's config which is v2's API. Tippy is the same.

I think instead we should encourage the authors to create their own wrapper APIs around Popper which would allow them to update Popper majors in the future easily without breaking the API. This is much more intuitive with v3's lower-level API anyways.

atomiks avatar Nov 23 '21 05:11 atomiks

We do support 'auto' IIRC, see the options table.

TBH, I didn't follow the popper changes closely, and it took us a while to sort out the issues from the 2.x upgrade. We don't have a lot of people around who are familiar with this part of the codebase, so if you could help us with any upgrades, I'd really appreciate it.

/CC @GeoSot since he spent some time with the popper code.

XhmikosR avatar Nov 23 '21 08:11 XhmikosR

I can only tell, we make use that placement: 'auto' in docs and in our code

https://github.com/twbs/bootstrap/blob/5290080d4df3047d04c8a232bca5dc7f8eaa4bc6/js/src/tooltip.js#L52-L58

GeoSot avatar Nov 26 '21 10:11 GeoSot