Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

CSS: Add animation-iteration-count to cssNumber jquery/jquery#2792#2793

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
ufologist wants to merge2 commits intojquery:masterfromufologist:master

Conversation

ufologist
Copy link
Contributor

@jquerybot
Copy link

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visithttp://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information,check the status of your CLA check.

@ufologist
Copy link
ContributorAuthor

I only want to fix a litter bug, why not?

CLA Verification Results

The name ufologist requires manual verification.

I have done below

  1. Signing the CLA
  2. Updating Git Config
  3. git commit --amend --reset-author
  4. git push -f origin master

@mgol
Copy link
Member

Is ufologist your full name? The CLA check requires a full name (first & last) unless someone has a reason to hide it.

@ufologist
Copy link
ContributorAuthor

Thanks@mgol
ufologist is my nickname, but even I config and sign my full name, it does not work too.

CLA Verification Results

ufologist : Name on signature (xxx) doesn't match name on commit.

I need more help. :(

@mgol
Copy link
Member

@ufologist Your commits must be signed by your full name as well. You can modify it via:

git commit --amend --author "FirstName LastName <email@address>"

You have 3 commits, though, every one of them would have to be modified. You might want to squash them to one commit first.

@mgol
Copy link
Member

@jquery/core This is a mess, we're adding more & more stuff to this array and it'll always be a problem as CSS is expanding. I'd like to get rid of thispx-appending but it might be too big a breaking change.

This is probably too late for 3.0 but how about switching the blacklist to a whitelist? If jQuery doesn't appendpx for you it's easier to fix by yourself than if it does append erroneously. My guess would be 99% of the uses are common properties like(min-|max-|)(height|width) ortop,left etc. We could identify those most popular ones and appendpx only to them. Migrate could restore the previous behavior but warn if someone passes a numeric value to properties that are not on the current blacklist.

@markelog
Copy link
Member

Would you mind creating another issue about it? Otherwise we might clutter this one.

@mgol
Copy link
Member

Would you mind creating another issue about it? Otherwise we might clutter this one.

Right, created#2795 for that.

@mgol
Copy link
Member

I'm looking at the added tests and the ones before it and they seem wrong... We're checking for truthiness of$div.css( "aProperty" ) just after we set it but if jQuery was erroneously addingpx to values for this property then the browser would reject it and set an empty string which is falsy; this would lead us to incorrectly treat the browser as not supporting the property and, as a result, pass the test. Shouldn't we check:

$div.css("aProperty")!==undefined

instead?

@ufologist
Copy link
ContributorAuthor

Thanks@mgol

I understand that set 'animation-iteration-count' by css() is not a common use.
But if not anyone find the same problem except me?

My guess would be 99% of the uses are common properties like (min-|max-|)(height|width) or top, left etc.

I accpet that

warn if someone passes a numeric value to properties that are not on the current blacklist.

Than I will know 'animation-iteration-count' property need added tojQuery.cssNumber by myself, or I can docss('animation-iteration-count', '2') passby the rule(use a string instead a numeric).

@markelog
Copy link
Member

@ufologist Why this was closed?

I don't think@mgol comment was implying that we don't want your changes.

@mgol
Copy link
Member

This PR is fine, I proposed changing an approach but that's a separate duscussion. This PR just needs changes I mentioned in my last comment. I'm reopening.

@mgolmgol reopened thisDec 31, 2015
@@ -860,6 +860,15 @@ QUnit.test( "Do not append px (#9548, #12990)", function( assert ) {
} else {
assert.ok( true, "No support for column-count CSS property" );
}

$div.css( "animation-iteration-count", 2 );
Copy link
Member

Choose a reason for hiding this comment

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

@ufologist Would you like to change this line (and a few previous ones) according to what I wrote in#2793 (comment)? I think it would be good to merge after that's done.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks@mgol.
I have pass the CLA and change the test with your suggestion.

@timmywiltimmywil added this to the1.12/2.2 milestoneJan 4, 2016
@mgolmgol self-assigned thisJan 4, 2016
@mgolmgol removed this from the1.12/2.2 milestoneJan 7, 2016
@mgolmgol closed this indf822caJan 7, 2016
mgol pushed a commit that referenced this pull requestJan 7, 2016
mgol pushed a commit that referenced this pull requestJan 7, 2016
@mgol
Copy link
Member

mgol commentedJan 7, 2016

Landed ondf822ca (master),b9a6958 (2.2-stable) &01fb17b (1.12-stable). Thanks!

@locklockbot locked asresolvedand limited conversation to collaboratorsJan 18, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers
No reviews
Assignees

@mgolmgol

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@ufologist@jquerybot@mgol@markelog@timmywil

[8]ページ先頭

©2009-2025 Movatter.jp