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

tooltip/popover: add a customClass option #31834

Merged
merged 3 commits into from Nov 20, 2020

Conversation

remeika
Copy link

@remeika remeika commented Oct 5, 2020

Please squash before merging

Fixes #19415

Add extra classes to Tooltips and Popovers using the additionalClasses option, using $.addClass() behind the scenes.

Details:

  • Additional classes are evaluated and added every time the show() command is run. This should be idempotent, and allows the class list to be updated every time the component is shown.
  • New classes are added after the component template is hydrated in the DOM; if classes are added to a custom template and using additionalClasses, both will show up.

Questions I still have:

  • I targeted this release at the 4.X branch. I am not sure if I did the right thing there, rather than basing my work off main.
  • It looks like the file size of 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?

@remeika remeika requested a review from a team as a code owner October 5, 2020 05:55
@remeika remeika force-pushed the additional-classes-on-popover branch 2 times, most recently from bd44a28 to 49656c5 Compare October 5, 2020 06:31
@XhmikosR
Copy link
Member

XhmikosR commented Oct 5, 2020

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.

js/src/tooltip.js Outdated Show resolved Hide resolved
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.

Nice work @remeika! 👍

Some changes to do but not so much

@@ -41,6 +41,7 @@ const DefaultType = {
container: '(string|element|boolean)',
fallbackPlacement: '(string|array)',
boundary: '(string|element)',
additionalClasses: '(string|function)',
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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

js/src/tooltip.js Outdated Show resolved Hide resolved
js/tests/unit/popover.js Outdated Show resolved Hide resolved
js/tests/unit/tooltip.js Outdated Show resolved Hide resolved
js/tests/unit/tooltip.js Outdated Show resolved Hide resolved
@cankitm
Copy link
Contributor

cankitm commented Oct 9, 2020

But we'd need a patch for the main branch anyway. About bundlewatch, leave that to us, we'll update the file before merging.

@XhmikosR @Johann-S Can I pick the patch for main branch?

@XhmikosR
Copy link
Member

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?

@Johann-S
Copy link
Member

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

@XhmikosR
Copy link
Member

XhmikosR commented Nov 5, 2020

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

@Johann-S
Copy link
Member

it's a point of view, or we can add it when folks ask for it 🤷

@XhmikosR
Copy link
Member

@remeika can you make a PR for main too please?

@XhmikosR XhmikosR added this to Inbox in v4.6.0 via automation Nov 14, 2020
@XhmikosR XhmikosR force-pushed the additional-classes-on-popover branch 2 times, most recently from 01e34d1 to 3aa544b Compare November 14, 2020 15:02
@XhmikosR XhmikosR changed the title Add extra classes to tooltips and popovers Add an customClass option to the Tooltip component. Nov 14, 2020
@XhmikosR XhmikosR force-pushed the additional-classes-on-popover branch from 3aa544b to 7527113 Compare November 14, 2020 15:05
@XhmikosR XhmikosR linked an issue Nov 14, 2020 that may be closed by this pull request
@XhmikosR XhmikosR changed the title Add an customClass option to the Tooltip component. Add a customClass option to the Tooltip component. Nov 16, 2020
@XhmikosR XhmikosR changed the title Add a customClass option to the Tooltip component. tooltip/popover: add a customClass option Nov 16, 2020
@XhmikosR XhmikosR force-pushed the additional-classes-on-popover branch from 7527113 to f667f0c Compare November 20, 2020 05:45
@XhmikosR
Copy link
Member

@Johann-S LGTY?

@rohit2sharma95 if we merge this, can you make a PR against main later please? 🙂

@rohit2sharma95
Copy link
Collaborator

Okay sure 👍 @XhmikosR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v4.6.0
Shipped
Development

Successfully merging this pull request may close these issues.

Simplify the customization of popover
5 participants