Restore offset option for dropdown component

[joke2k]

With Popper 2.0 the dropdown offset option was removed from bootstrap but remains in the documentation and in our ❤️ s.
This is an humble PR to restore them.

Fixes #32418

[XhmikosR]

My problem is that I have no idea why it was removed in the first place, and if there was some special reason...

[joke2k]

My problem is that I have no idea why it was removed in the first place, and if there was some special reason...

It think because popper changes the way to configure the offset.
I used the old tests to check. But is not fully-coverage yet

[rohit2sharma95]

Thanks for the PR 🙂 @joke2k
I have left a few comments, it is better to have the offset option in the configuration.

[rohit2sharma95]

@joke2k Can you make the update, please?

[rohit2sharma95]

It is now complete for dropdown though but the same should be done for Tooltip/Popover also, IMO.
Ping @XhmikosR 🙂

[javieraviles]

If I'm not wrong, scrollspy offset is under the same use-case. In alpha v5 is not working either.

[joke2k]

If I'm not wrong, scrollspy offset is under the same use-case. In alpha v5 is not working either.

I checked into bs4 and bs5 but ScrollSpy component never used Popper for its offset.
In beta1 Scrollspy offset option works pretty well

https://jsfiddle.net/yhv62bz1/

[XhmikosR]

@rohit2sharma95 this seems to be duplicated. Might be worth checking at a later time if we it makes sense to deduplicate this. Lower priority right now :)

[rohit2sharma95]

Yes, should tackle it in a new PR later?

[joke2k]

I'm trying to deduplicate this into utils/index.js and I was wondering if we can do the same for _getPopperConfig...
but the function signature becoming a little bit weird:

const getPopperOffsets = (element, config) => {
  if (typeof offset === 'string') {
    return offset.split(',').map(val => Number.parseInt(val, 10))
  }

  if (typeof offset === 'function') {
    return popperData => offset(popperData, element)
  }

  return offset
}

const getPopperConfig = (element, placement, offset, modifiers, options) => {
  const defaultBsConfig = {
    placement: placement,
    modifiers: [...(modifiers || [])],
    ...(options || {})
  }

  if (offset) {
    defaultBsConfig.modifiers.push({
      name: 'offset',
      options: {
        offset: getPopperOffsets(element, offset)
      }
    })
  }

  return defaultBsConfig
}

the element is necessary because offset configured as function needs it for the second parameter.

[XhmikosR]

No need to do this now. We'll have plenty of time after this lands :)

[javieraviles]

If I'm not wrong, scrollspy offset is under the same use-case. In alpha v5 is not working either.

I checked into bs4 and bs5 but ScrollSpy component never used Popper for its offset.
In beta1 Scrollspy offset option works pretty well

https://jsfiddle.net/yhv62bz1/

I thought the offset would not only be applied when scrolling manually to detect which button should be highlighted but also when clicking on the button to have such offset applied in the scroll. My bad

[rohit2sharma95]

I also notice if someone wants to pass the offset option (Or any other modifier for Popper) via the popperConfig option, the other modifiers are replaced by the array of the modifiers passed in the popperConfig.

So if the user passes only one modifier in the array of popperConfig, other modifiers are reset to the defaults used by Popper.

bootstrap/js/src/dropdown.js

Lines 318 to 322 in 86802f7

	return {
	...popperConfig,
	...this._config.popperConfig
	}
	}

So few options should be maintained individually by Bootstrap that are customized by bootstrap. For example, the default value for fallbackPlacements is changed by Bootstrap. And the offset option is one of them.

And it should be mentioned in the docs that if someone passes the configuration via popperConfig, it is kind of handling the Popper's configuration natively because it can override some of the default configurations by bootstrap.

/CC @XhmikosR @MartijnCuppens

[mdo]

@rohit2sharma95 @XhmikosR Should we open an issue or PR to track that separately so we can merge this?

[rohit2sharma95]

Yes, either the docs should be updated or the way of merging the popperConfig in the plugins.