Add amp-img srcset linter check

[tharders]

No description provided.

[caroqliu]

Looks great. Could be worth also adding warn cases for layout = fill, flex-item, and intrinsic.

Are there any additional considerations for nested amp-img in the case where the inner element is provided as a fallback?

[tharders]

@caroqliu
The check ignored the "fixed" and "fixed-height" layouts, so "fill" and the others where already checked.
But actually "nodisplay" was also checked. So I switched the logic and now explicitly check "fill", "flex-item", "intrinsic" and "responsive".
I also changed the messages from "responsive" to "non-fixed layout" so that there is no confusion with the "responsive" layout mode.

I did not think about nested amp-img before.
Thank you very much for this hint!
This check will cover all amp-img tags regardless of their nesting. But now it will check for the layout of the parent element if it is an amp- element.
So if the parent is fixed the amp-img does not need to have a srcset.
But I must admit that I am not sure if this approach is correct / enough in all cases.

[caroqliu]

Sorry I missed this the first time -- could be helpful to also provide which amp-img elements are affected, similar to how PSI is able to do:

[CLAassistant]

All committers have signed the CLA.

[tharders]

@caroqliu I have changed the check so that it will emit a warning for each image.
Please have a look at the changed message

[caroqliu]

@caroqliu I have changed the check so that it will emit a warning for each image.
Please have a look at the changed message

I'm not seeing a new commit since the last review, am I missing something?

[tharders]

@caroqliu oops, the push hook failed.
Changes are pushed now

[caroqliu]

Sorry, I should have clarified -- this looks like it is emitting a warning for every image that is missing a srcset. It may be more manageable to send a single warning that lists all the images that are missing a srcset, similar to the images missing alt text:

amp-toolbox/packages/linter/src/rules/ImagesHaveAltText.ts

Lines 24 to 32 in 25a4df3

	for (let key in imgsWithoutAlt) {
	imgsWithoutAlt[key] > 1
	? (output += key + color(" [used " + imgsWithoutAlt[key] + " times]\n"))
	: (output += key + "\n");
	}
	return Object.keys(imgsWithoutAlt).length > 0
	? this.warn("Missing alt text from images: \n" + output)
	: this.pass();

[caroqliu]

I'm open to emitting multiple warnings if that sounds like a better approach to you, what do you think?

[tharders]

There are all possible approaches in the different linter tests, so there is no "right" way to do this.
Some checks emit one warning without details to the findings, some emit one warning with details to the findings like you suggested and some emit a warning for each finding.

If we ever want to show the results in the frontend I think a list of warnings is a better approach.
We might be able to add other metadata to each entry that can be used to display a nice list like in your screenshot.
With only one warning entry this will never be possible.