Skip to content

Rewritten tooltip and popover without jquery#24014

Merged
Johann-S merged 1 commit intotwbs:v4-without-jqueryfrom
alekitto:tooltip-no-jquery
Sep 25, 2017
Merged

Rewritten tooltip and popover without jquery#24014
Johann-S merged 1 commit intotwbs:v4-without-jqueryfrom
alekitto:tooltip-no-jquery

Conversation

@alekitto
Copy link
Copy Markdown
Contributor

Tooltip and popover rewritten without jquery

The event system has been modified to allow registering the same handler for more than one event, delegate events in .one method and unregistering one-off handlers through .off
The delegateTarget property has been added to the event object, replacing the jquery currentTarget (native events' currentTarget property is read-only)

@Johann-S
Copy link
Copy Markdown
Member

Did you update our event system because you had issue with it ? Because currently we are able to register for more than one event on the same element

Comment thread js/src/dom/eventHandler.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

change not relevant here, please keep the affectation here

@alekitto
Copy link
Copy Markdown
Contributor Author

Before this, the same handler function cannot be registered and unregistered correctly for more than one element or event.
This would lead to mouseover event to be not unregistered on touch-enabled device (in tooltip.js:383)

That's why I removed the uidEvent setting in getUidEvent function, which is called with the original handler function as first argument in the on method

Comment thread js/src/util.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you changed that again 😆

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Argh! This was an old branch, it seems that i've messed up this in a rebase..

Comment thread js/src/tooltip.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

noop should be added to our Util object

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Comment thread js/src/tooltip.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

.on method explicitly checks for empty string, but selector can be undefined or null
Probably the check event handler should be modified

Comment thread js/src/tooltip.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

same here why ?

@twbs twbs deleted a comment from alekitto Sep 20, 2017
@Johann-S
Copy link
Copy Markdown
Member

Can you make some tests on IE, on your computer ? because currently we cannot run our Saucelabs tests on folks PRs 😢

@alekitto
Copy link
Copy Markdown
Contributor Author

I've successfully executed the testsuite on chrome, chrome canary, firefox, safari, ie 11 and ms edge. Manual visual tests seem ok.

Comment thread js/src/util.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

noop should return an empty function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

noop should be an empty function, but ESLint does not allow empty functions

Copy link
Copy Markdown
Member

@Johann-S Johann-S Sep 21, 2017

Choose a reason for hiding this comment

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

Disable that rule just for this line ;)
with // eslint-disable-next-line no-empty-function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment thread js/tests/unit/dom/eventHandler.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you should use notOk here because it's a failed case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

Comment thread js/src/tooltip.js
_getConfig(config) {
config = $.extend(
if (typeof config !== 'undefined' &&
typeof config.container === 'object' && config.container.jquery) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is it config.container.jquery ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Checks if config.container is a jquery object. In that case i'll get the dom element via [0] .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that's right, I forgot about that property 😄

@Johann-S Johann-S merged commit 76ee411 into twbs:v4-without-jquery Sep 25, 2017
@Johann-S Johann-S mentioned this pull request Sep 25, 2017
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants