Skip to content

Warn about jQuery.cssNumber #296

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.

Sign up for GitHub

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

Jump to bottom
Closed
dmethvin opened this issue Mar 19, 2018 · 10 comments · Fixed by #337
Closed

Warn about jQuery.cssNumber #296

dmethvin opened this issue Mar 19, 2018 · 10 comments · Fixed by #337

Comments

@dmethvin
Copy link
Member

See jquery/jquery#4009

Detect assignments to jQuery.cssNumber and recommend that it only be done if that property exists; note that it's going away in 4.0.

Recommend that people do not pass raw numbers to .css() and instead always pass strings and append "px" when appropriate. Point out line-height as a troublesome case.
@dmethvin dmethvin added this to the 3.2.0 milestone May 7, 2019
dmethvin added a commit to dmethvin/jquery-migrate that referenced this issue Jun 9, 2019
@dmethvin
CSS: Warn and fill jQuery.cssNumber and .css(name, Number)
aee3d06 
Fixes jquery#296
@dmethvin
Copy link
Member Author

dmethvin commented Jun 9, 2019

Fixed in aee3d06

@dmethvin dmethvin closed this as completed Jun 9, 2019
@mgol
Copy link
Member

mgol commented Sep 2, 2019

@dmethvin Where did that commit land? I don't see it on master... I was about to submit an issue about filling cssNumber and I saw this issue.

@dmethvin
Copy link
Member Author

dmethvin commented Sep 3, 2019

Darned good question! I can't see it either. I'll reopen this to get it into master.

@dmethvin dmethvin reopened this Sep 3, 2019
dmethvin added a commit to dmethvin/jquery-migrate that referenced this issue Sep 4, 2019
@dmethvin
CSS: Warn and fill jQuery.cssNumber and .css(name, Number)
7900560 
Fixes jquery#296
@dmethvin dmethvin mentioned this issue Sep 8, 2019
Closed
@dmethvin dmethvin mentioned this issue Oct 10, 2019
Merged
mgol pushed a commit to jquery/jquery that referenced this issue Oct 21, 2019
@dmethvin @mgol
Offset: Send px-ed strings to .css()
57038fa 
An upcoming release of Migrate will generate warnings for calls to .css() that pass numbers rather than strings, see jquery/jquery-migrate#296 . At the moment, core's .offset() setter passes numbers rather than px strings so it would throw warnings. This commit fixes that.

Closes gh-4508
@dmethvin dmethvin removed this from the 3.2.0 milestone Mar 25, 2020
@dmethvin
Copy link
Member Author

This can't be landed until internal cssNumber usages are removed. I don't think that is going to happen until 4.0.

@mgol
Copy link
Member

mgol commented Mar 25, 2020

@dmethvin Oh, I see, the linked commit won't work for jQuery 3.x. That said, it seems we could do something else & detect & warn against pushes to cssNumber? That way Core code would work.

@dmethvin
Copy link
Member Author

Hmmm, sure, we could use a set hook like we did with jQuery.cssProps, I think that should work here as well.

@mgol
Copy link
Member

mgol commented Apr 1, 2020

Hmm, now that I think about it, pushes to jQuery.cssNumber are one thing but passing numbers as values to the .css() setter is another and the latter one is most likely way more common. Also, the former one is impossible to detect correctly as we want to keep allowing people to add stuff there, but just to check first if jQuery.cssNumber is defined.

Is the issue that Core itself uses numbers as values somewhere? Can you share more details?

@mgol
Copy link
Member

mgol commented Apr 1, 2020 •
edited
Loading

Hmm, looking closer at the commit removing cssNumber (jquery/jquery@00a9c2e) it seems we should change the order of some checks in jQuery 3.x so that cssNumber is only checked if you provide a number value. Maybe that would make it feasible to warn on getter access.

mgol added a commit to mgol/jquery that referenced this issue Apr 1, 2020
@mgol @dmethvin
Offset: Send px-ed strings to .css()
36fa086 
An upcoming release of Migrate will generate warnings for calls to `.css()`
that pass numbers rather than strings, see jquery/jquery-migrate#296. At
the moment, core's `.offset()` setter passes numbers rather than px strings
so it would throw warnings. This commit fixes that.

Ref jquerygh-4508

Co-authored-by: Dave Methvin <dave.methvin@gmail.com>
@mgol mgol mentioned this issue Apr 1, 2020
Closed
3 tasks
mgol added a commit to mgol/jquery that referenced this issue Apr 1, 2020
@mgol @dmethvin
Offset: Send px-ed strings to .css()
d4a4b21 
An upcoming release of Migrate will generate warnings for calls to `.css()`
that pass numbers rather than strings, see jquery/jquery-migrate#296. At
the moment, core's `.offset()` setter passes numbers rather than px strings
so it would throw warnings. This commit fixes that.

Fixes jquerygh-4525
Ref jquerygh-4508

Co-authored-by: Dave Methvin <dave.methvin@gmail.com>
@mgol
Copy link
Member

mgol commented Apr 1, 2020

@dmethvin I think your PR might work with minor modifications. I submitted jquery/jquery#4651 so that it also works with Core master.

mgol pushed a commit to mgol/jquery-migrate that referenced this issue Apr 7, 2020
@dmethvin @mgol
CSS: Warn and fill jQuery.cssNumber and .css(name, Number)
02bd06c 
Fixes jquery#296
mgol pushed a commit to mgol/jquery-migrate that referenced this issue Apr 9, 2020
@dmethvin @mgol
CSS: Warn and fill jQuery.cssNumber and .css(name, Number)
8933053 
Fixes jquery#296
mgol pushed a commit to mgol/jquery-migrate that referenced this issue Apr 10, 2020
@dmethvin @mgol
CSS: Warn and fill jQuery.cssNumber and .css(name, Number)
a96d8ef 
Fixes jquery#296
@mgol mgol mentioned this issue Apr 10, 2020
Merged
@mgol
Copy link
Member

mgol commented Apr 10, 2020

@dmethvin PR: #337.

mgol pushed a commit to mgol/jquery-migrate that referenced this issue Apr 11, 2020
@dmethvin @mgol
CSS: Warn and fill jQuery.cssNumber and .css(name, Number)
d04d9b5 
Fixes jquery#296
@mgol mgol closed this as completed in #337 Apr 14, 2020
mgol added a commit that referenced this issue Apr 14, 2020
@mgol @dmethvin
CSS: Warn and fill .css(name, Number)
0d0d370 
Fixes #296
Closes #337

Co-authored-by: Dave Methvin <dave.methvin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

CSS: Warn and fill .css(name, Number) mgol/jquery-migrate
2 participants
@dmethvin @mgol