twbs
Expose strategy popper config in dropdown
Contains change and docs. I only did dropdown, but if wanted, the same copy could be moved to tooltips and popovers
Basically, the idea to expose few options from the config of the Popper was that those options were either customized by the bootstrap (mostly) or there was any other specific reason. IMO it is better to have a minimum number of options to maintain in the core plugin (until that's really required). :slightly_smiling_face:
The strategy option is neither customized by the bootstrap nor it is usually required. For these kinds of requirements, there is an option popperConfig, that can be used to pass the strategy option as well.
I understand your points.
My counterpoint is, how do you pass popperconfig in the data attribute without writing JavaScript?
Are we open to having the drop down config check for decoding a string into json, then it could work, but otherwise the downside to me is, if you want to change strategy, you have to write JS.
If we deserialize json then it wouldn’t expose any new options, meeting one of your objectives, and allow passing popperconfig, which is just extended onto the base bootstrap options, so that works great.
You guys open to this?
Sent from my iPhone
On May 28, 2021, at 1:46 AM, Rohit Sharma @.***> wrote:
Basically, the idea to expose few options from the config of the Popper was that those options were either customized by the bootstrap (mostly) or there was any other specific reason. IMO it is better to have a minimum number of options to maintain in the core plugin (until that's really required). 🙂
The strategy option is neither customized by the bootstrap nor it is usually required. For these kinds of requirements, there is an option popperConfig, that can be used to pass the strategy option as well.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.
Are we open to having the drop down config check for decoding a string into json, then it could work, but otherwise the downside to me is, if you want to change strategy, you have to write JS.
If I'm following right, this seems worthwhile to me.
OK so I created PR https://github.com/twbs/bootstrap/pull/34259 for parsing JSON in the data attribute for data-bs-popper-config Some clarifying questions can be found under that PR
Shortly, there may be any solution to accept the options for the popper configuration dynamically. That makes it possible to omit the requirement of the stringified JSON in the data attributes.
For now, it is okay to move this PR further IMO :slightly_smiling_face:
I duplicated the strategy parameter over to "tooltip" component for uniformity
@GeoSot @mdo Would this be merged if we brought it back up to main branch?
Made a new PR in the hopes it could be merged: https://github.com/twbs/bootstrap/pull/35947
@GeoSot @julien-deramond I'd really like to see this merged. Is there anything I can do to help this along? Specifically, I'd be happy to open a new PR with merge conflicts resolved and see it through the review process.
@GeoSot @julien-deramond I'd really like to see this merged. Is there anything I can do to help this along? Specifically, I'd be happy to open a new PR with merge conflicts resolved and see it through the review process.
I haven't looked at the PR, but I've rebased it by resolving the conflicts and changed the documentation to the Markdown format. @nwalters512, by reading the discussion, this PR would need at least some unit tests. You can probably create a PR targeting this branch by adding these unit tests, and anything that should be updated, or missing from your POV.
@julien-deramond I'd be happy to add some unit tests, but as far as I can tell, I can't open a PR that targets a branch on a fork. This is what I see when I try to open such a PR, 719media/bootstrap doesn't show up as a "base repository":
I'll provide my changes below as a patch file, but I'd also be happy to open a branch-new PR targeting main of this repo. Let me know if you'd prefer I do that.
From 740f0a9edec0e05d573ed2be6ed972b43dc693de Mon Sep 17 00:00:00 2001
From: Nathan Sarang-Walters <[email protected]>
Date: Wed, 7 Aug 2024 09:41:42 -0700
Subject: [PATCH] Add tests for popper strategy config
---
js/tests/unit/dropdown.spec.js | 52 ++++++++++++++++++++++++++++++++++
js/tests/unit/tooltip.spec.js | 38 +++++++++++++++++++++++++
2 files changed, 90 insertions(+)
diff --git a/js/tests/unit/dropdown.spec.js b/js/tests/unit/dropdown.spec.js
index 63ae4bd10..772c76573 100644
--- a/js/tests/unit/dropdown.spec.js
+++ b/js/tests/unit/dropdown.spec.js
@@ -178,6 +178,58 @@ describe('Dropdown', () => {
}))
expect(popperConfig.placement).toEqual('left')
})
+
+ it('should reflect strategy option in the constructor', () => {
+ return new Promise(resolve => {
+ fixtureEl.innerHTML = [
+ '<div class="dropdown">',
+ ' <button class="btn dropdown-toggle" data-bs-toggle="dropdown">Dropdown</button>',
+ ' <div class="dropdown-menu">',
+ ' <a class="dropdown-item" href="#">Link</a>',
+ ' </div>',
+ '</div>'
+ ].join('')
+
+ const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
+ const dropdown = new Dropdown(btnDropdown, {
+ strategy: 'fixed'
+ })
+
+ btnDropdown.addEventListener('shown.bs.dropdown', () => {
+ const dropdownMenu = document.querySelector('.dropdown-menu')
+ expect(dropdownMenu).not.toBeNull()
+ expect(dropdownMenu.computedStyleMap().get('position').value).toEqual('fixed')
+ resolve()
+ })
+
+ dropdown.toggle()
+ })
+ })
+
+ it('should reflect strategy option in data attribute', () => {
+ return new Promise(resolve => {
+ fixtureEl.innerHTML = [
+ '<div class="dropdown">',
+ ' <button class="btn dropdown-toggle" data-bs-toggle="dropdown" data-bs-strategy="fixed">Dropdown</button>',
+ ' <div class="dropdown-menu">',
+ ' <a class="dropdown-item" href="#">Link</a>',
+ ' </div>',
+ '</div>'
+ ].join('')
+
+ const btnDropdown = fixtureEl.querySelector('[data-bs-toggle="dropdown"]')
+ const dropdown = new Dropdown(btnDropdown)
+
+ btnDropdown.addEventListener('shown.bs.dropdown', () => {
+ const dropdownMenu = document.querySelector('.dropdown-menu')
+ expect(dropdownMenu).not.toBeNull()
+ expect(dropdownMenu.computedStyleMap().get('position').value).toEqual('fixed')
+ resolve()
+ })
+
+ dropdown.toggle()
+ })
+ })
})
describe('toggle', () => {
diff --git a/js/tests/unit/tooltip.spec.js b/js/tests/unit/tooltip.spec.js
index 37f2c230d..022bc9d42 100644
--- a/js/tests/unit/tooltip.spec.js
+++ b/js/tests/unit/tooltip.spec.js
@@ -959,6 +959,44 @@ describe('Tooltip', () => {
tooltip.show()
})
})
+
+ it('should reflect strategy option in the constructor', () => {
+ return new Promise(resolve => {
+ fixtureEl.innerHTML = '<a href="#" rel="tooltip" title="Another tooltip">'
+
+ const tooltipEl = fixtureEl.querySelector('a')
+ const tooltip = new Tooltip(tooltipEl, {
+ strategy: 'fixed'
+ })
+
+ tooltipEl.addEventListener('shown.bs.tooltip', () => {
+ const tip = document.querySelector('.tooltip')
+ expect(tip).not.toBeNull()
+ expect(tip.computedStyleMap().get('position').value).toEqual('fixed')
+ resolve()
+ })
+
+ tooltip.show()
+ })
+ })
+
+ it('should reflect strategy option in data attribute', () => {
+ return new Promise(resolve => {
+ fixtureEl.innerHTML = '<a href="#" rel="tooltip" title="Another tooltip" data-bs-strategy="fixed"></a>'
+
+ const tooltipEl = fixtureEl.querySelector('a')
+ const tooltip = new Tooltip(tooltipEl)
+
+ tooltipEl.addEventListener('shown.bs.tooltip', () => {
+ const tip = document.querySelector('.tooltip')
+ expect(tip).not.toBeNull()
+ expect(tip.computedStyleMap().get('position').value).toEqual('fixed')
+ resolve()
+ })
+
+ tooltip.show()
+ })
+ })
})
describe('hide', () => {
--
2.46.0
Yeah, I suppose in this case you'd have to fork https://github.com/719media/bootstrap, start from the patch-9, and then create a PR but IDK with what target. Maybe it can't work like that 🤷
Nevermind, I've committed your tests via https://github.com/twbs/bootstrap/pull/34120/commits/eac65566dac5a0827afa894c844a45ab4f359c7f and kept authorship with the "Co-authored-by" mention in the commit message. If no extra-code is needed for this branch, maybe we can continue doing that so we don't lose previous commits and authorship from the OP.
Thanks a lot for your work on the tests. I've removed the "needs tests" label, and this PR can wait for a review from JS maintainers maybe for a v5.4.0.