Added support for capturing asset in multiple dpr and width combination #1536
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ti_dpr_width_capture
chinmay-browserstack
requested review from
pankaj443 and
shantanuk-browserstack
chinmay-browserstack
changed the base branch from
master
to
capture_srcset_support
Amit3200
reviewed
Feb 28, 2024
packages/core/src/snapshot.js
Outdated
| @@ -118,6 +118,7 @@ function getSnapshotOptions(options, { config, meta }) { | |||
| disableCache: config.discovery.disableCache, | |||
| captureMockedServiceWorker: config.discovery.captureMockedServiceWorker, | |||
| captureSrcset: config.discovery.captureSrcset, | |||
| captureResponsiveAssets: config.discovery.captureResponsiveAssets, | |||
There was a problem hiding this comment.
This should not be needed, we should call this explicitly again if there is enableJS true. If we want to keep it then we need confirmation from product.
There was a problem hiding this comment.
Ideally we should do this automatically.
ninadbstack
reviewed
Feb 28, 2024
packages/core/src/discovery.js
Outdated
| @@ -138,11 +139,24 @@ function processSnapshotResources({ domSnapshot, resources, ...snapshot }) { | |||
| return { ...snapshot, resources }; | |||
| } | |||
|
|
|||
| async function responsiveDomParams(snapshot, client) { | |||
There was a problem hiding this comment.
Not something we want to call per snapshot, move it to the snapshot config validation and cache the response in percy.js so that it can be applied to all snapshots in current build
ninadbstack
reviewed
Mar 4, 2024
packages/client/test/client.test.js
Outdated
| }); | ||
|
|
||
| it('gets device details', async () => { | ||
| api.reply('/device-details', () => [200, { data: '<<device-data-without-build-id>>' }]); |
There was a problem hiding this comment.
Suggested change
| api.reply('/device-details', () => [200, { data: '<<device-data-without-build-id>>' }]); | |
| api.reply('/device-details', () => [200, { data: ['<<device-data-without-build-id>>' ]}]); |
ninadbstack
reviewed
Mar 4, 2024
packages/core/src/discovery.js
Outdated
| @@ -317,7 +324,8 @@ export function createDiscoveryQueue(percy) { | |||
| try { | |||
| yield* captureSnapshotResources(page, snapshot, { | |||
| captureWidths: !snapshot.domSnapshot && percy.deferUploads, | |||
| capture: callback | |||
| capture: callback, | |||
| captureResponsiveDom: percy.deviceDetails || [] | |||
ninadbstack
reviewed
Mar 4, 2024
packages/core/src/discovery.js
Outdated
| @@ -198,6 +198,16 @@ async function* captureSnapshotResources(page, snapshot, options) { | |||
| } | |||
| } | |||
|
|
|||
| if (captureResponsiveDom) { | |||
| for (const device of captureResponsiveDom) { | |||
| yield waitForDiscoveryNetworkIdle(page, discovery); | |||
There was a problem hiding this comment.
Suggested change
| yield waitForDiscoveryNetworkIdle(page, discovery); | |
| // We are not adding these widths and pixels ratios in loop above because we want to explicitly reload the page after resize which we dont do above | |
| yield waitForDiscoveryNetworkIdle(page, discovery); |
ninadbstack
reviewed
Mar 4, 2024
packages/core/src/discovery.js
Outdated
| }); | ||
| if (discovery.devicePixelRatio) { | ||
| const log = logger('core:discovery'); | ||
| log.warn('discovery.devicePixelRatio is deprecated percy will now auto capture resource in all devicePixelRatio, Ignoring configuration'); |
There was a problem hiding this comment.
Suggested change
| log.warn('discovery.devicePixelRatio is deprecated percy will now auto capture resource in all devicePixelRatio, Ignoring configuration'); | |
| log.deprecate('discovery.devicePixelRatio is deprecated percy will now auto capture resource in all devicePixelRatio, Ignoring configuration'); |
Amit3200
reviewed
Mar 4, 2024
| try { | ||
| let url = 'device-details'; | ||
| if (buildId) url += `?build_id=${buildId}`; | ||
| const { data } = await this.get(url); |
There was a problem hiding this comment.
This returns list only correct?
ninadbstack
approved these changes
Mar 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added new flag
captureResponsiveAssetswhich will capture assets in all dpr, width combination supported by current project