-
Notifications
You must be signed in to change notification settings - Fork 37
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
[New sniff] Discourage CSS rules for images setting width:auto #137
Comments
This issue is problematic to sniff for automatically in a reliable way as rules are applied with |
The rules to check end with |
We are just talking images in the post area? |
No, we are talking about CSS rules that affect images. See screenshot above for an example of how an icon can get totally messed up using |
I saw that, but I think theme authors need to test their css and act accordingly. |
The problem with checking this is that some images can have certain classes on them, and then they would have Looks like more of a visual regression test than code sniff to me. |
The purpose of the sniff is to find the cases it can find and issue a warning. Obviously, it can't find all cases. But I have seen a lot of themes that have done this, and it is always with |
Here's some sample code pulled from some gallery-specific code used in nearly every one of my themes:
Because you're missing the context of the rest of the CSS code as well as no visual check, you have no way of knowing whether that code is doing what it should be doing. It works as it should, by the way. That's just one use case that I pulled up in a matter of minutes. I'm sure I can find many, many more. There are far too many legit use cases where setting I do agree that seeing |
I think you would find that removing the I suggest this sniff, as a warning, to catch the cases that can be caught, and to educate authors that haven't ever tested with user defined widths (for |
I can guarantee you that it won't be fine in some scenarios. That's why I specifically wrote the code and have been refining it over the course of 10 years now. |
I don't understand the resistance to having a sniff that gives a warning on a visual thing. We can't automate the unit test, but we can suggest that certain things are potential visual problems, just like other warnings that must be checked manually. What is the big deal if authors actually learn something, and have fewer support tickets? It makes better themes. |
That would fall under visual regression test then. |
Any idea what should we do with this? |
I stand by the fact that if the sniff can be written, it will help the author catch some problems they might not notice in their limited testing. |
Two points about this:
Having said that, to even attempt to write sniffs for this, we'd need a lot of code samples, so please add any and all code samples to this issue. |
To me this seems like it would maybe be better to include in Theme Sniffer. We could see if this can be linted using The Removal of CSS tokenizer in PHPCS 4.0 is one more reason why I'm against writing sniff for this (no long term gain imo). |
Let's not forget that, while probably not as high priority as checking CSS files, a sniff for checking inline HTML within PHP files for this should probably still be written. |
I don't think this should be included, we do not review design unless the page is broken / unreadable. To me this would fall under "not pretty". |
It really is not about design at all. It's about the user being able to get the size as coded in the HTML, and the browser blowing up small images like icons. |
Triage resolution: we'll leave this for the sniffer. Sniffing CSS is super hard without context, and CSS in either PHP or HTML can be tricky to sniff if the value is in the variable. |
Rule type:
Warning
Rule:
Flag CSS rules for images that have the property
width:auto
.There is no rule in the Handbook for this, but it falls under the general guideline to honor the user's choices.
The default value for CSS width is auto, so there is no need to specify it. When it is specified, it will override the HTML width attribute. So an
img
tag with a different width attribute than the source image is would be affected, and it will display as the size of the source image instead of the size the user requested.Decision needed:
Good wording for something that is not a requirement. This is more of admonition to "play nice".
Notes for implementation:
div
.To do:
The text was updated successfully, but these errors were encountered: