Skip to content

refactor(plugins): query elements without jquery#26645

Merged
Johann-S merged 1 commit into
v4-devfrom
v4-dev-jo-improve-queried-elements
Jun 3, 2018
Merged

refactor(plugins): query elements without jquery#26645
Johann-S merged 1 commit into
v4-devfrom
v4-dev-jo-improve-queried-elements

Conversation

@Johann-S

@Johann-S Johann-S commented Jun 1, 2018

Copy link
Copy Markdown
Member

Fixes: #26643

/CC @XhmikosR

@Johann-S Johann-S force-pushed the v4-dev-jo-improve-queried-elements branch from 64ca404 to aff677b Compare June 1, 2018 12:52
@Johann-S

Johann-S commented Jun 1, 2018

Copy link
Copy Markdown
Member Author

I have some issues to fix for IE 😆

EDIT:
Ok :scope isn't supported by IE, we added a polyfill in v4-without-jquery to make it works, so I'll remove those changes for tab.js

@Johann-S Johann-S force-pushed the v4-dev-jo-improve-queried-elements branch from aff677b to 39d92ab Compare June 1, 2018 15:04
@Johann-S Johann-S requested a review from XhmikosR June 1, 2018 15:05
Comment thread js/src/collapse.js
this._element.style[dimension] = 0

if (this._triggerArray.length > 0) {
if (this._triggerArray.length) {

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.

I'd keep the length checks

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's useless because 0 is a false value

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep but that rule isn't activated

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.

I never said it was :P I remember I made the length changes some time ago, so I'd say keep them even though we don't have that check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I prefer to remove them, it's part of the JavaScript power 💪

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.

I don't agree, because I'd rather be explicit when possible. It's actually one of the problems of JavaScript, not strict types etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That's a well known property of JavaScript that 0 is a false value, be explicit isn't a good thing when it's hide a language particularity because other languages are different

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.

Sorry, I don't agree with the length removal. Even if we know that, I always prefer to be explicit.

The rest of the patch looks good, though. 👍

@XhmikosR XhmikosR left a comment

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.

Forgot to approve for the rest.

@Johann-S Johann-S force-pushed the v4-dev-jo-improve-queried-elements branch from 39d92ab to 160f672 Compare June 2, 2018 11:55
@Johann-S Johann-S merged commit a79b8aa into v4-dev Jun 3, 2018
@Johann-S Johann-S deleted the v4-dev-jo-improve-queried-elements branch June 3, 2018 09:40
@mdo mdo mentioned this pull request Jun 3, 2018
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