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.

Sign up for GitHub

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

Jump to bottom

Offset option for dropdown can be function #24222

Merged
merged 7 commits into from
Oct 3, 2017
Merged

Offset option for dropdown can be function #24222

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9f9d33
Offset option can be function (Popper.js)
Oct 3, 2017
b1ba188
Fix...add function type for offset option
Oct 3, 2017
ed3dd31
Remove constants for popper config
Oct 3, 2017
9be393b
Optimize code. Remove foreach loop.
Oct 3, 2017
a3ce6f4
Merge branch 'v4-dev' into v4-dev
Johann-S Oct 3, 2017
0ddc9aa
Refactoring. Remove getOffset method
Oct 3, 2017
d46a0b1
Merge branch 'v4-dev' of https://github.com/romanlex/bootstrap into v…
Oct 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/4.0/components/dropdowns.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,7 @@ Options can be passed via data attributes or JavaScript. For data attributes, ap
<tbody>
<tr>
<td>offset</td>
<td>number | string</td>
<td>number | string | function</td>
<td>0</td>
<td>Offset of the dropdown relative to its target. For more information refer to Popper.js's <a href="https://popper.js.org/popper-documentation.html#modifiers..offset.offset">offset docs</a>.</td>
</tr>
Expand Down
17 changes: 13 additions & 4 deletions js/src/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const Dropdown = (() => {
}

const DefaultType = {
offset : '(number|string)',
offset : '(number|string|function)',
flip : 'boolean'
}

Expand Down Expand Up @@ -241,17 +241,26 @@ const Dropdown = (() => {
return placement
}

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

_detectNavbar() {
return $(this._element).closest('.navbar').length > 0
}

_getPopperConfig() {
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 •
edited
Loading

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

offsetConf.offset = this._config.offset
}
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
    }

const popperConfig = {
placement : this._getPlacement(),
modifiers : {
offset : {
offset : this._config.offset
},
offset : offsetConf,
flip : {
enabled : this._config.flip
}
Expand Down