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

Offset option for dropdown can be function #24222

Merged
merged 7 commits into from
Oct 3, 2017
Merged

Conversation

romanlex
Copy link
Contributor

@romanlex romanlex commented Oct 3, 2017

Fix #24208

Offset option can be function (Popper.js)

Add support for calculate and dynamically change offset position for dropdown menu.
Function call always when call Popper.js modifierFn.

With this code I can change any dropdown position as on picture

const dropdownMegaToggle = $('.dropdown-toggle--mega');
dropdownMegaToggle.dropdown({
    offset: function(data) {
        let popper = data.popper;
        let reference = data.reference;
        let w = $(window).width();
        let buttonOffset = dropdownMegaToggle.offset();
        popper.top = 100;
        reference.width = w / 2; // f*cking Popper.js crutch
        popper.left = w / 2 - popper.width / 2 - buttonOffset.left;
        return data
    }
});

2017-10-03 13-37-50

Fix : #24223

@@ -39,6 +39,8 @@ const Dropdown = (() => {
const ARROW_DOWN_KEYCODE = 40 // KeyboardEvent.which value for down arrow key
const RIGHT_MOUSE_BUTTON_WHICH = 3 // MouseEvent.which value for the right button (assuming a right-handed mouse)
const REGEXP_KEYDOWN = new RegExp(`${ARROW_UP_KEYCODE}|${ARROW_DOWN_KEYCODE}|${ESCAPE_KEYCODE}`)
const POPPER_OFFSET_KEY = 'offset'
const POPPER_OFFSET_FN_KEY = 'fn'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan to add new const for that 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, const is redundant. Updated

let key = POPPER_OFFSET_KEY
if (typeof this._config.offset === 'function') {
key = POPPER_OFFSET_FN_KEY
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO something like that would be enough :

const offsetConf = {}
if (typeof this._config.offset === 'function') {
  offsetConf.offsetFn = (data) => this._getOffset(data)
} else {
  offsetConf.offset = this._config.offset
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your. It's better solution. Const is redundant

_getPopperConfig() {
      const offsetConf = {}
      if (typeof this._config.offset === 'function') {
        offsetConf.fn = (data) => this._getOffset(data)
      } else {
        offsetConf.offset = this._config.offset
      }
      const popperConfig = {
        placement : this._getPlacement(),
        modifiers : {
          offset : offsetConf,
          flip : {
            enabled : this._config.flip
          }
        }
      }

      // Disable Popper.js for Dropdown in Navbar
      if (this._inNavbar) {
        popperConfig.modifiers.applyStyle = {
          enabled: !this._inNavbar
        }
      }
      return popperConfig
    }

@@ -241,16 +243,28 @@ const Dropdown = (() => {
return placement
}

_getOffset(data) {
const popperOffset = $.extend({}, data.offsets, this._config.offset(data.offsets) || {})
Object.keys(popperOffset).forEach((key) => {
Copy link
Member

@Johann-S Johann-S Oct 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to ask I don't know well offset function from Popper.js but why do you need that ?

Ok I read a bit and IMO if you just do return $.extend({}, data.offsets, this._config.offset(data.offsets) || {}) it will be enough here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data object has many another keys. We need to change only data.offsets and not other. If we return only extended object with data.offsets - Popper is broken

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right 👍 but instead of a Foreach you just have to do :
data.offsets = popperOffset

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Agree :) Sorry don't know how I can stick new changes in this PR romanlex@9be393b

Copy link
Member

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @romanlex to tackled that 👍

const offsetConf = {}
if (typeof this._config.offset === 'function') {
offsetConf.fn = (data) => this._getOffset(data)
} else {
Copy link
Member

@Johann-S Johann-S Oct 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking maybe we should remove _getOffset method because that's not something we will use outside of this and remplaced it by :

offsetConf.fn = (data) => {
     data.offsets = $.extend({}, data.offsets, this._config.offset(data.offsets) || {})
     return data
}

Can you give it a try ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this method but in my opinion this is less readable. On the other hand, the method is not needed, because obviously not planned to manage offseting inside boostrap

@Johann-S Johann-S merged commit 527f55c into twbs:v4-dev Oct 3, 2017
@mdo mdo mentioned this pull request Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants