Skip to content

docs: Update Playwright config example#2897

Merged
sarah11918 merged 6 commits intowithastro:mainfrom
rstacruz:patch-1
Mar 29, 2023
Merged

docs: Update Playwright config example#2897
sarah11918 merged 6 commits intowithastro:mainfrom
rstacruz:patch-1

Conversation

@rstacruz
Copy link
Copy Markdown
Contributor

What kind of changes does this PR include?

  • Minor content fixes (broken links, typos, etc.)

Description

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 21, 2023

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a9bd740
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/642490b3ed403c0008df0cf6
😎 Deploy Preview https://deploy-preview-2897--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@sarah11918
Copy link
Copy Markdown
Member

Thanks for this PR! I can see in the Playwright documentation they do have a different example config. I'm going to ping @chuckcarpenter since this was their original content based on their custom setup for a second pair of eyes here!

Chuck, any chance you can confirm that you've made these updates to your own setup, too? Or whether this is better/more recent general advice?

@sarah11918 sarah11918 added the code snippet update Updates a code sample: typo, outdated code etc. label Mar 22, 2023

export default defineConfig({
webServer: {
command: 'yarn preview',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@sarah11918, not entirely related to this PR, but do we 'recommend' yarn to run the commands here? Or should we just fall to PNPM or NPM which we use most in docs (imo)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great call out, Elian! This advanced user case was submitted by @chuckcarpenter as the setup they used. I know earlier in the page, for basic usage, we do include our normal package manager tabs for options. We do typically use NPM generically, all other things being equal. So I'd certainly be open to that change!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ElianCodes Would you be willing to make the suggested change here so I can just commit it and not have to go through the effort of manually adding you as a co-author to this PR? 😅

@ElianCodes
Copy link
Copy Markdown
Member

@sarah11918 if it helps, I've verified the suggested changes in my own playwright config on elian.codes. The suggested changes are all valid!

This is my config:

import { defineConfig } from '@playwright/test';

export default defineConfig({
  testDir: './tests',
  use: {
    baseURL: 'http://localhost:3000',
  },
  outputDir: 'test-results/',
  webServer: {
    command: 'pnpm preview',
    port: 3000,
  },
});

@chuckcarpenter
Copy link
Copy Markdown
Contributor

@sarah11918 sorry for the late response. Yeah, this LGTM based on changes I've also made on projects.

Copy link
Copy Markdown
Member

@ElianCodes ElianCodes left a comment

Choose a reason for hiding this comment

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

Left changes to switch from yarn to npm

sarah11918 and others added 2 commits March 29, 2023 16:14
Co-authored-by: Elian ☕️ <hello@elian.codes>
Co-authored-by: Elian ☕️ <hello@elian.codes>
Co-authored-by: Elian ☕️ <hello@elian.codes>
@sarah11918
Copy link
Copy Markdown
Member

sarah11918 commented Mar 29, 2023

Looking great, gonna swap a couple more yarns that I think are still hanging around, then it's in the merge queue!

(Looks like the only ones left are in package manager tabs, so that's fine!)

Thanks @rstacruz for this helpful update! We appreciate you taking the time to submit this PR!

Kind thanks to @chuckcarpenter and @ElianCodes for jumping in and confirming here. Go Team!

@sarah11918 sarah11918 merged commit 5a1771f into withastro:main Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code snippet update Updates a code sample: typo, outdated code etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants