Skip to content

Conversation

@patrik-csak
Copy link
Contributor

It looks to me like the note is in the wrong place

Copilot AI review requested due to automatic review settings June 25, 2025 02:33
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR moves and consolidates the note about the snapshot path parameter to a more appropriate position in the documentation.

  • The relocated note now clearly explains that the name parameter accepts an array of path segments for snapshot files.
  • The duplicated note regarding snapshotName was removed to reduce redundancy.
Comments suppressed due to low confidence (1)

docs/src/test-snapshots-js.md:49

  • Ensure the use of name here (instead of snapshotName found in the removed note) is intentional and consistent with the rest of the documentation.
    > Note that `name` also accepts an array of path segments to the snapshot file such as `expect().toHaveScreenshot(['relative', 'path', 'to', 'snapshot.png'])`.

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, thank you! let's move it to a slightly different spot.


- `chromium-darwin` - the browser name and the platform. Screenshots differ between browsers and platforms due to different rendering, fonts and more, so you will need different snapshots for them. If you use multiple projects in your [configuration file](./test-configuration.md), project name will be used instead of `chromium`.

The snapshot name and path can be configured with [`property: TestConfig.snapshotPathTemplate`] in the playwright config.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the note should go under this section. otherwise it disrupts the enumaration flow, reads weird to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Skn0tt Skn0tt merged commit 56ccccd into microsoft:main Jun 26, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants