core(image-size-responsive): remove elidedUrl, elide url property instead #13226

prerana1821 · 2021-10-17T10:52:36Z

Summary

The audit "image-size-responsive" used to contain "elidedUrl" property. All other audits save the result of URL.elideDataURI to their url property. So, I have set url to this elided value, and droped elidedUrl. I also have updated all the tests related to this audit.

Resolved Issue

fixes #13182

google-cla · 2021-10-17T10:52:41Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

adamraine

Are you using your own linter to adjust everything? This PR is mostly style changes, some of which violate our existing linter rules.

Would it be possible to revert the style changes but keep the elidedUrl deprecation?

adamraine · 2021-10-18T15:50:15Z

lighthouse-core/audits/image-size-responsive.js

@@ -8,28 +8,29 @@
 *   the page are large enough with respect to the pixel ratio. The
 *   audit will list all visible images that are too small.
 */
-'use strict';
+"use strict";


We use '. yarn lint --fix should resolve these.

adamraine · 2021-10-18T16:02:13Z

lighthouse-core/test/results/sample_v2.json

@@ -9,7 +9,7 @@
      "axe-core": "4.2.3"
    }
  },
-  "lighthouseVersion": "8.6.0",
+  "lighthouseVersion": "8.5.1",


I don't think we want the version to change here.

google-cla · 2021-10-18T19:38:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla · 2021-10-18T19:45:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

prerana1821 · 2021-10-18T19:46:30Z

@adamraine I have done all the fixes mentioned.

adamraine · 2021-10-18T22:12:11Z

Thank you for making those fixes!

There are still some unrelated style changes in this PR (indentations, spacing, etc.). These changes just make the PR larger than it needs to be, especially in our generated files like sample_v2.json. Some of the style changes are good, but they still aren't really necessary.

TL;DR: If the changes aren't functional, we should leave them out of this PR :)

prerana1821 · 2021-10-19T20:17:38Z

Hey @adamraine , Thanks a lot for reviewing my changes. I have removed the not required changes.

adamraine

Thank you for reverting the style changes in sample_v2.json, but most of this PR is still pure style changes. I've highlighted the changes we should keep, but the rest should be left out :)

Once you make those changes, you can run yarn update:sample-json to fix sample_v2.json automatically (without style changes).

adamraine · 2021-10-19T22:31:15Z

lighthouse-core/test/audits/image-size-responsive-test.js

@@ -53,8 +53,11 @@ describe('Images: size audit', () => {
      });


All the changes in this file look like style, please only update any tests that are failing.

adamraine · 2021-10-19T22:35:12Z

lighthouse-core/test/fixtures/fraggle-rock/reports/sample-flow-result.json

-                  "failures": [
-                    "No manifest was fetched"
-                  ],
+                  "failures": ["No manifest was fetched"],


You shouldn't need to change this file. We will update it in the future, but for now it's fine.

adamraine · 2021-10-19T22:37:42Z

lighthouse-core/audits/image-size-responsive.js

  return {
-    url: image.src,
-    elidedUrl: URL.elideDataURI(image.src),
+    url: URL.elideDataURI(image.src),


Removing elidedUrl here is good.

adamraine · 2021-10-19T22:38:01Z

lighthouse-core/audits/image-size-responsive.js

@@ -45,7 +46,7 @@ const LARGE_IMAGE_FACTOR = 0.75;
 // considered SMALL.
 const SMALL_IMAGE_THRESHOLD = 64;

-/** @typedef {{url: string, elidedUrl: string, displayedSize: string, actualSize: string, actualPixels: number, expectedSize: string, expectedPixels: number}} Result */
+/** @typedef {{url: string, displayedSize: string, actualSize: string, actualPixels: number, expectedSize: string, expectedPixels: number}} Result */


This change is good.

You can run yarn update:sample-json to propagate these changes to sample_v2.json.

adamraine · 2021-10-19T22:52:03Z

lighthouse-core/audits/image-size-responsive.js

-      {key: 'displayedSize', itemType: 'text', text: str_(UIStrings.columnDisplayed)},
-      {key: 'actualSize', itemType: 'text', text: str_(UIStrings.columnActual)},
-      {key: 'expectedSize', itemType: 'text', text: str_(UIStrings.columnExpected)},
+      {key: 'url', itemType: 'url', text: str_(i18n.UIStrings.columnURL)},


Removal of the elidedUrl heading is good. We like to keep each heading entry on a single line though. Anything else I didn't highlihgt is a style change we don't need.

connorjclark · 2021-10-20T19:13:43Z

lighthouse-core/audits/image-size-responsive.js

@@ -69,8 +71,8 @@ function isVisible(imageRect, viewportDimensions) {
 */
 function isSmallerThanViewport(imageRect, viewportDimensions) {
  return (
-    (imageRect.bottom - imageRect.top) <= viewportDimensions.innerHeight &&


Your editor seems to have made multiple unnecessary formatting changes. Could you undo those?

You can run git diff origin/master to see the current diff between your current branch and master. You can remove all the unnecessary changes by running that command multiple times as you undo things.

adamraine · 2021-10-25T23:36:38Z

Make sure to run yarn update:sample-json and commit the changes to sample_v2.json :)

.

prerana1821 · 2021-10-28T18:00:47Z

@adamraine I'm facing few issues running that command on my machine.

adamraine · 2021-10-28T18:21:45Z

Hmm the command may be broken on windows, unfortunately I can't test on a windows machine at the moment to resolve this.

If you resolve the merge conflicts and give me access to your fork, I can run the command on my end.

prerana1821 · 2021-10-28T20:51:00Z

@adamraine Thanks a lot
I have resolved the merge conflicts & send you an invite for my forked repo

adamraine

Thanks @prerana1821!

fix: drop image size responsiveness

a288f9e

prerana1821 requested a review from a team as a code owner October 17, 2021 10:52

prerana1821 requested review from adamraine and removed request for a team October 17, 2021 10:52

vercel bot deployed to Preview October 17, 2021 10:52 View deployment

google-cla bot added the cla: no label Oct 17, 2021

devtools-bot assigned adamraine Oct 17, 2021

devtools-bot added the waiting4reviewer label Oct 17, 2021

prerana1821 mentioned this pull request Oct 17, 2021

drop image-size-responsive elidedUrl #13182

Closed

adamraine requested changes Oct 18, 2021

View reviewed changes

fix: formatting

76a43f4

vercel bot deployed to Preview October 18, 2021 19:38 View deployment

fix: lighthouse version

556a662

vercel bot deployed to Preview October 18, 2021 19:46 View deployment

fix: removed not required changes

7fd1522

vercel bot deployed to Preview October 19, 2021 20:16 View deployment

google-cla bot added cla: yes and removed cla: no labels Oct 19, 2021

adamraine reviewed Oct 19, 2021

View reviewed changes

connorjclark previously requested changes Oct 20, 2021

View reviewed changes

fix: undo formatting changes

5ad1b48

vercel bot deployed to Preview October 21, 2021 20:39 View deployment

fix: undo formatting changes

2fb2c6d

vercel bot deployed to Preview October 21, 2021 20:46 View deployment

vercel bot deployed to Preview October 22, 2021 12:21 View deployment

fix: space formatting changes

15441b8

vercel bot deployed to Preview October 22, 2021 12:22 View deployment

fix: space formatting changes

9ee2fd0

vercel bot deployed to Preview October 22, 2021 12:23 View deployment

fix: formatting changes

4585874

vercel bot deployed to Preview October 22, 2021 12:24 View deployment

fix: formatting changes

7f8a238

vercel bot deployed to Preview October 22, 2021 12:25 View deployment

fix: merge conflicts

832df61

vercel bot deployed to Preview October 28, 2021 20:06 View deployment

fix: formatting changes

15d397d

vercel bot deployed to Preview October 28, 2021 20:11 View deployment

fix: formatting changes

9faa284

vercel bot deployed to Preview October 28, 2021 20:12 View deployment

update sample

e8e3c7f

vercel bot deployed to Preview October 28, 2021 21:04 View deployment

nit

e427b17

vercel bot deployed to Preview October 28, 2021 21:30 View deployment

adamraine approved these changes Oct 28, 2021

View reviewed changes

sample

80ba2ba

vercel bot deployed to Preview October 28, 2021 21:43 View deployment

adamraine merged commit 4da2012 into GoogleChrome:master Oct 29, 2021

adamraine deleted the drop-image-size-responsive branch October 29, 2021 17:50

connorjclark mentioned this pull request Oct 29, 2021

☂️ Breaking changes for v9 #12614

Closed

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(image-size-responsive): remove elidedUrl, elide url property instead #13226

core(image-size-responsive): remove elidedUrl, elide url property instead #13226

prerana1821 commented Oct 17, 2021 •

edited by paulirish

Loading

google-cla bot commented Oct 17, 2021

adamraine left a comment

adamraine Oct 18, 2021

adamraine Oct 18, 2021

google-cla bot commented Oct 18, 2021

google-cla bot commented Oct 18, 2021

prerana1821 commented Oct 18, 2021

adamraine commented Oct 18, 2021

prerana1821 commented Oct 19, 2021

adamraine left a comment

adamraine Oct 19, 2021

adamraine Oct 19, 2021

adamraine Oct 19, 2021

adamraine Oct 19, 2021

adamraine Oct 21, 2021

adamraine Oct 19, 2021

connorjclark Oct 20, 2021

connorjclark Oct 20, 2021

adamraine commented Oct 25, 2021

prerana1821 commented Oct 28, 2021

adamraine commented Oct 28, 2021

prerana1821 commented Oct 28, 2021

adamraine left a comment

		@@ -53,8 +53,11 @@ describe('Images: size audit', () => {
		});

core(image-size-responsive): remove elidedUrl, elide url property instead #13226

core(image-size-responsive): remove elidedUrl, elide url property instead #13226

Conversation

prerana1821 commented Oct 17, 2021 • edited by paulirish Loading

google-cla bot commented Oct 17, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Oct 18, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

google-cla bot commented Oct 18, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

prerana1821 commented Oct 18, 2021

adamraine commented Oct 18, 2021

prerana1821 commented Oct 19, 2021

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented Oct 25, 2021

prerana1821 commented Oct 28, 2021

adamraine commented Oct 28, 2021

prerana1821 commented Oct 28, 2021

adamraine left a comment

Choose a reason for hiding this comment

prerana1821 commented Oct 17, 2021 •

edited by paulirish

Loading