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

Support Popper virtual elements #32376

Merged
merged 11 commits into from Dec 21, 2020
Merged

Support Popper virtual elements #32376

merged 11 commits into from Dec 21, 2020

Conversation

septatrix
Copy link
Contributor

@septatrix septatrix commented Dec 8, 2020

Fixes #32365

@septatrix septatrix requested a review from a team as a code owner December 8, 2020 10:29
@XhmikosR XhmikosR changed the title Support Popper virtual elements (fixes #32365) Support Popper virtual elements Dec 8, 2020
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Dec 8, 2020
js/src/dropdown.js Outdated Show resolved Hide resolved
@alecpl
Copy link
Contributor

alecpl commented Dec 8, 2020

I think getBoundingClientRect is available in all supported browsers, no?

@XhmikosR
Copy link
Member

XhmikosR commented Dec 8, 2020

I think getBoundingClientRect is available in all supported browsers, no?

Yup. We already make use of it in the codebase.

@XhmikosR XhmikosR marked this pull request as draft December 8, 2020 12:08
Adds the ability to use objects implementing the virtual element interface as the value for the reference option of a dropdown config.
@septatrix septatrix marked this pull request as ready for review December 8, 2020 13:36
@septatrix
Copy link
Contributor Author

I think getBoundingClientRect is available in all supported browsers, no?

Yes but if you refer to the check that is due to getBoundingClientRect being required for popper virtual elements which can be arbitrary objects as long as the provide this method.

@alecpl
Copy link
Contributor

alecpl commented Dec 8, 2020

@septatrix nevermind my comment, I've now learned what this is about ;) Sorry for noise.

@septatrix
Copy link
Contributor Author

I tidied up the commit history a bit.

I also thought about adding an example using a virtual element but I do not think it is worthwhile as it would be longer than most other examples and most most of the other options do not really have examples as well. However if you also would want to add an example I would suggest something like I drafted in this codepen which uses virtual elements to provide a custom contextmenu at the cursor position. (The codepen uses some trickery to make bootstrap think the passed object is an element as it does not use a patched version.)

@septatrix
Copy link
Contributor Author

@XhmikosR should I rebase them again to squash your commits into the previous ones or do PRs get squashed when merging? I only read in the contribution guidelines that you would like to have a clean commit log :D

@XhmikosR
Copy link
Member

All good, we can squash when merging.

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta3 via automation Dec 14, 2020
@XhmikosR XhmikosR removed this from Inbox in v5.0.0-beta2 Dec 14, 2020
@XhmikosR XhmikosR removed this from Inbox in v5.0.0-beta3 Dec 15, 2020
@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta2 via automation Dec 15, 2020
@XhmikosR XhmikosR moved this from Inbox to Review in v5.0.0-beta2 Dec 15, 2020
@rohit2sharma95
Copy link
Collaborator

But the reference that is passed in the configuration is not being used for the reference of Popper 🤔
This code is still the same:

let referenceElement = this._element
if (this._config.reference === 'parent') {
referenceElement = parent
} else if (isElement(this._config.reference)) {
referenceElement = this._config.reference
// Check if it's jQuery element
if (typeof this._config.reference.jquery !== 'undefined') {
referenceElement = this._config.reference[0]
}
}

So even if you pass the reference as an object, it does not make any change in the dropdown.
@septatrix 🙂

@septatrix
Copy link
Contributor Author

Fixed

Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

If you add the .dropdown-menu-end class in the .dropdown-menu, the position of the dropdown is wrong (position is not as per the passed coordinates). But that's not the issue of Popper. Works well without .dropdown-menu-end.

js/tests/unit/dropdown.spec.js Outdated Show resolved Hide resolved
@septatrix
Copy link
Contributor Author

If you add the .dropdown-menu-end class in the .dropdown-menu, the position of the dropdown is wrong (position is not as per the passed coordinates). But that's not the issue of Popper. Works well without .dropdown-menu-end.

What exactly do you mean with wrong. It is now to the left of the passed coordinates as long as there is space for it which is what I would expect. Some bad positioning happens at e.g. lower screen edges but that also happens to normal dropdowns.

@septatrix
Copy link
Contributor Author

septatrix commented Dec 16, 2020

Some bad positioning happens at e.g. lower screen edges but that also happens to normal dropdowns.

Nevermind. It just changes to left positioning so that it is centered above the cursor so that is working as intended i guess.

I did however notice a misbehaving placement in a few cases but that may have been fixed on main by now.

Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Maybe in the future, tooltip/popover can also use this virtual element feature to follow the mouse cursor.

v5.0.0-beta2 automation moved this from Review to Approved Dec 21, 2020
typeof config.reference.getBoundingClientRect !== 'function'
) {
// Popper virtual elements require a getBoundingClientRect method
throw new Error(`${NAME}: Option "reference" provided type "object" without a required "getBoundingClientRect" method.`)
Copy link
Member

Choose a reason for hiding this comment

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

Should we change this to a TypeError instead of the generic Error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, missed it 😄
But then it should also be TypeError 🤔

throw new Error(
`${componentName.toUpperCase()}: ` +
`Option "${property}" provided type "${valueType}" ` +
`but expected type "${expectedTypes}".`)
}

Copy link
Member

Choose a reason for hiding this comment

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

OK, let's leave it as is for now and we tackle it later.

@XhmikosR XhmikosR merged commit 2d46e47 into twbs:main Dec 21, 2020
v5.0.0-beta2 automation moved this from Approved to Done Dec 21, 2020
sijusamson pushed a commit to sijusamson/bootstrap that referenced this pull request Jan 4, 2021
Adds the ability to use objects implementing the virtual element interface as the value for the reference option of a dropdown config.

Co-authored-by: XhmikosR <xhmikosr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta2
  
Done
Development

Successfully merging this pull request may close these issues.

Support Popper v2 virtual element
4 participants