-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
tooltip/popover: add a customClass
option
#31834
Conversation
bd44a28
to
49656c5
Compare
Thanks for the PR! That being said, we always target the main branch first and then we backport anything if deemed necessary. That being said, since the tests differ, we'd need a v4-dev patch anyway. But we'd need a patch for the main branch anyway. About bundlewatch, leave that to us, we'll update the file before merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @remeika! 👍
Some changes to do but not so much
js/src/tooltip.js
Outdated
@@ -41,6 +41,7 @@ const DefaultType = { | |||
container: '(string|element|boolean)', | |||
fallbackPlacement: '(string|array)', | |||
boundary: '(string|element)', | |||
additionalClasses: '(string|function)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a parameter called : customClass
which would allow an array of string or a function it would be better if someone wants to add a lot of classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Johann-S As this PR stands now, the additionalClasses property is a logic-less wrapper around jQuery.addClass()
. As of jquery 3.3, addClass() does accept an array of classes. So my initial thinking was that in Bootstrap 4.X, we guarantee that a string or function are valid inputs (although passing in an array will work in environments with a modern version of jQuery), and then in Bootstrap 5 we could rely on a more modern version of jQuery. But I just checked package.json
on the main
branch, and jQuery is no longer listed as a peer dependency. Will Bootstrap 5 not depend on jQuery? If so I will definitely change my strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in v4 we can stay like you did, just the renaming (additionalClasses -> customClass) but since v5 is intended to work without jQuery I would prefer an array and a function
07387d7
to
8548610
Compare
My only concern is that people might ask for the ability add custom classes in all components. So, shouldn't we have a helper function and add this functionality in all components? |
yup you're right it will be asked for sure. But currently our code architecture doesn't allow to have one helper shared across our components because we don't use inheritance |
Maybe we should wait until this can be applied to all components? I feel like it's sort of incomplete if we don't add this for all components |
it's a point of view, or we can add it when folks ask for it 🤷 |
@remeika can you make a PR for main too please? |
01e34d1
to
3aa544b
Compare
customClass
option to the Tooltip component.
3aa544b
to
7527113
Compare
customClass
option to the Tooltip component.customClass
option to the Tooltip component.
customClass
option to the Tooltip component.customClass
option
7527113
to
f667f0c
Compare
@Johann-S LGTY? @rohit2sharma95 if we merge this, can you make a PR against main later please? 🙂 |
Okay sure 👍 @XhmikosR |
Porting of #31834 to main. Co-authored-by: XhmikosR <[email protected]>
Please squash before merging
Fixes #19415
Add extra classes to Tooltips and Popovers using the
additionalClasses
option, using$.addClass()
behind the scenes.Details:
show()
command is run. This should be idempotent, and allows the class list to be updated every time the component is shown.additionalClasses
, both will show up.Questions I still have:
main
.bootstrap.bundle.js
has been locked in place: adding any additional code causes the build pipeline to fail. I'm wondering what course of action an individual contributor should take in this situation: Is the 4.X branch feature-locked? Should I try to delete some code elsewhere to try to squeeze under the limit? Should I just raise the limit?