ronantakizawa/codereview-bench huggingface dataset:
Repository: freeCodeCamp/freeCodeCamp
File: e2e/ms-trophy-show.spec.ts (TypeScript)

Code before:
const askForHelpButton = translations.buttons['ask-for-help'];

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/foundational-c-sharp-with-microsoft/write-your-first-code-using-c-sharp/trophy-write-your-first-code-using-c-sharp'
  );
});

test.afterEach(async ({ page }) => {
  await page.close();
});

test('the page should render with correct title', async ({ page }) => {
  await expect(page).toHaveTitle(
    'Write Your First Code Using C# - Trophy - Write Your First Code Using C# | Learn | freeCodeCamp.org'
  );
});

test('Correct Verify Trophy button', async ({ page }) => {
  const askHelp = page.getByTestId('verify-trophy-button');
  await expect(askHelp).toBeVisible();
  await expect(askHelp).toContainText(verifyTrophy);
});

test('Correct Ask for help button', async ({ page }) => {
  const checkAnswer = page.getByTestId('ask-for-help-button');
  await expect(checkAnswer).toBeVisible();
  await expect(checkAnswer).toContainText(askForHelpButton);
});


Review comment (suggestion):
```suggestion
  const checkAnswer = page.getByRole('button', { name: askForHelpButtonText })
    ).toBeVisible();
```

Code after:
const askForHelpButtonText = translations.buttons['ask-for-help'];

test.beforeEach(async ({ page }) => {
  await page.goto(
    '/learn/foundational-c-sharp-with-microsoft/write-your-first-code-using-c-sharp/trophy-write-your-first-code-using-c-sharp'
  );
});

test.afterEach(async ({ page }) => {
  await page.close();
});

test('the page should render with correct title', async ({ page }) => {
  await expect(page).toHaveTitle(
    'Write Your First Code Using C# - Trophy - Write Your First Code Using C# | Learn | freeCodeCamp.org'
  );
});

test('Correct Verify Trophy button', async ({ page }) => {
  const askHelpButton = page.getByRole('button', {
    name: verifyTrophyButtonText
  });
  await expect(askHelpButton).toBeVisible();
  await expect(askHelpButton).toHaveText(verifyTrophyButtonText);
});

test('Correct Ask for help button', async ({ page }) => {
  const checkAnswerButton = page.getByRole('button', {
    name: askForHelpButtonText
  });
  await expect(checkAnswerButton).toBeVisible();
  await expect(checkAnswerButton).toContainText(askForHelpButtonText);
});
Repository: freeCodeCamp/freeCodeCamp
File: e2e/profile.spec.ts (TypeScript)

Code before:
import { test, expect, type Page } from '@playwright/test';

const profilePageElements = {
  camper: 'camper-profile',
  portfolio: 'portfolio-projects'
} as const;

const certs = [
  'Responsive Web Design',
  'JavaScript Algorithms and Data Structures',
  'Front End Development Libraries',
  'Data Visualization',
  'Relational Database',
  'Back End Development and APIs',
  'Quality Assurance',
  'Scientific Computing with Python',
  'Data Analysis with Python',
  'Information Security',
  'Machine Learning with Python',
  'College Algebra with Python',
  'Foundational C# with Microsoft'
];

const legacyCerts = [
  'Front End',
  'Back End',

Review comment (suggestion):
```suggestion
import { test, expect } from '@playwright/test';
```

Remove the import because...

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const certs = [
  {
    name: 'Responsive Web Design',
    url: '/certification/certifieduser/responsive-web-design'
  },
  {
    name: 'JavaScript Algorithms and Data Structures',
    url: '/certification/certifieduser/javascript-algorithms-and-data-structures'
  },
  {
    name: 'Front End Development Libraries',
    url: '/certification/certifieduser/front-end-development-libraries'
  },
  {
    name: 'Data Visualization',
    url: '/certification/certifieduser/data-visualization'
  },
  {
    name: 'Relational Database',
    url: '/certification/certifieduser/relational-database-v8'
  },
  {
Repository: freeCodeCamp/freeCodeCamp
File: e2e/profile.spec.ts (TypeScript)

Code before:
import { test, expect } from '@playwright/test';

const profilePageElements = {
  camper: 'camper-profile',
  portfolio: 'portfolio-projects'
} as const;

const certs = [
  'Responsive Web Design',
  'JavaScript Algorithms and Data Structures',
  'Front End Development Libraries',
  'Data Visualization',
  'Relational Database',
  'Back End Development and APIs',
  'Quality Assurance',
  'Scientific Computing with Python',
  'Data Analysis with Python',
  'Information Security',
  'Machine Learning with Python',
  'College Algebra with Python',
  'Foundational C# with Microsoft'
];

const legacyCerts = [
  'Front End',
  'Back End',
  'Data Visualization',
  'Information Security and Quality Assurance',
  'Full Stack'
];


Review comment (suggestion):
We shouldn't have type casting here. Are you seeing any TypeScript errors?

```suggestion
};
```

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const certs = [
  {
    name: 'Responsive Web Design',
    url: '/certification/certifieduser/responsive-web-design'
  },
  {
    name: 'JavaScript Algorithms and Data Structures',
    url: '/certification/certifieduser/javascript-algorithms-and-data-structures'
  },
  {
    name: 'Front End Development Libraries',
    url: '/certification/certifieduser/front-end-development-libraries'
  },
  {
    name: 'Data Visualization',
    url: '/certification/certifieduser/data-visualization'
  },
  {
    name: 'Relational Database',
    url: '/certification/certifieduser/relational-database-v8'
  },
  {
    name: 'Back End Development and APIs',
    url: '/certification/certifieduser/back-end-development-and-apis'
  },
  {
    name: 'Quality Assurance',
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/close-button/close-button.tsx (TypeScript)

Code before:
import React from 'react';

export interface CloseButtonProps {
  className?: string;
  label?: string;
  onClick: () => void;
}

/**
 * Basic UI component for closing modals, alerts, etc.
 */
export function CloseButton({
  className,
  label,
  onClick
}: CloseButtonProps): JSX.Element {
  const classes = [
    // Remove browser's default styles
    'bg-transparent',
    'border-none',
    // Text styles
    'text-lg',
    'font-bold',
    'text-foreground-primary',
    // Active state
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus:ring',
    'focus:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

Review comment (suggestion):
The original implementation has `text-xl` as the text size, but we don't have this value in the config:

https://github.com/freeCodeCamp/freeCodeCamp/blob/4282dd5bda71856b92b0f5d3c3fbe7f97787efa4/tools/ui-components/tailwind.config.js#L89-L95

I'm using `text-lg` for now, but we might want to add another text size to the list as the current text size in /learn is 28px:

<img width="1680" alt="Screenshot 2023-10-09 at 23 23 54" src="https://github.com/freeCodeCamp/freeCodeCamp/assets/25715018/f70a381a-a1ac-4be3-adf5-f6bb8c8153f3">


Code after:
import React from 'react';

export interface CloseButtonProps {
  className?: string;
  label?: string;
  onClick: () => void;
}

/**
 * Basic UI component for closing modals, alerts, etc.
 */
export function CloseButton({
  className,
  label,
  onClick
}: CloseButtonProps): JSX.Element {
  const classes = [
    // Remove browser's default styles
    'bg-transparent',
    'border-none',
    // Text styles
    'text-lg',
    'font-bold',
    'text-foreground-primary',
    // Active state
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus-visible:ring',
    'focus-visible:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/close-button/close-button.tsx (TypeScript)

Code before:
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus:ring',
    'focus:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

  return (
    <button className={classes} onClick={onClick} type='button'>
      <span className='sr-only'>{label ?? 'Close'}</span>
      <span aria-hidden>×</span>
    </button>
  );
}


Review comment (suggestion):
We might want to revisit this. We either need to add `react-i18next` to the library for the translation of the text, or we make `label` a required prop, and have the consumer pass the translated string to the component.

IMHO, we should go with the former since there could be cases where having fixed / fallback text in the component library make more sense.

Code after:
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus-visible:ring',
    'focus-visible:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

  return (
    <button className={classes} onClick={onClick} type='button'>
      <span className='sr-only'>{label ?? 'Close'}</span>
      <span aria-hidden>×</span>
    </button>
  );
}
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/close-button/close-button.tsx (TypeScript)

Code before:

/**
 * Basic UI component for closing modals, alerts, etc.
 */
export function CloseButton({
  className,
  label,
  onClick
}: CloseButtonProps): JSX.Element {
  const classes = [
    // Remove browser's default styles
    'bg-transparent',
    'border-none',
    // Text styles
    'text-lg',
    'font-bold',
    'text-foreground-primary',
    // Active state
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus:ring',
    'focus:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

  return (
    <button className={classes} onClick={onClick} type='button'>
      <span className='sr-only'>{label ?? 'Close'}</span>
      <span aria-hidden>×</span>
    </button>
  );
}


Review comment (suggestion):
These values aren't applied.
```suggestion
```

about the active state in this button, I think it will be weird, because the button will close the element when it's activated, so :active shouldn't be there.

Code after:

/**
 * Basic UI component for closing modals, alerts, etc.
 */
export function CloseButton({
  className,
  label,
  onClick
}: CloseButtonProps): JSX.Element {
  const classes = [
    // Remove browser's default styles
    'bg-transparent',
    'border-none',
    // Text styles
    'text-lg',
    'font-bold',
    'text-foreground-primary',
    // Active state
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus-visible:ring',
    'focus-visible:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

  return (
    <button className={classes} onClick={onClick} type='button'>
      <span className='sr-only'>{label ?? 'Close'}</span>
      <span aria-hidden>×</span>
    </button>
  );
}
Repository: freeCodeCamp/freeCodeCamp
File: tools/ui-components/src/close-button/close-button.tsx (TypeScript)

Code before:
export function CloseButton({
  className,
  label,
  onClick
}: CloseButtonProps): JSX.Element {
  const classes = [
    // Remove browser's default styles
    'bg-transparent',
    'border-none',
    // Text styles
    'text-lg',
    'font-bold',
    'text-foreground-primary',
    // Active state
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus:ring',
    'focus:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

  return (
    <button className={classes} onClick={onClick} type='button'>
      <span className='sr-only'>{label ?? 'Close'}</span>
      <span aria-hidden>×</span>
    </button>
  );
}


Review comment (suggestion):
We are using `:focus-visible` in `/learn` for these buttons, I will update the button component in https://github.com/freeCodeCamp/freeCodeCamp/pull/51443
```suggestion
    'focus-visible:ring',
    'focus-visible:ring-focus-outline-color',
```

Code after:
export function CloseButton({
  className,
  label,
  onClick
}: CloseButtonProps): JSX.Element {
  const classes = [
    // Remove browser's default styles
    'bg-transparent',
    'border-none',
    // Text styles
    'text-lg',
    'font-bold',
    'text-foreground-primary',
    // Active state
    'active:before:w-full',
    'active:before:h-full',
    'active:before:absolute',
    'active:before:inset-0',
    'active:before:border-3',
    'active:before:border-transparent',
    'active:before:bg-gray-900',
    'active:before:opacity-20',
    // Focus state
    'focus:outline-none', // Hide the default browser outline
    'focus-visible:ring',
    'focus-visible:ring-focus-outline-color',
    // Content positioning
    'flex',
    'justify-center',
    'items-center',
    // Others
    'w-[24px]',
    'h-[24px]',
    'opacity-50',
    className
  ].join(' ');

  return (
    <button className={classes} onClick={onClick} type='button'>
      <span className='sr-only'>{label ?? 'Close'}</span>
      <span aria-hidden>×</span>
    </button>
  );
}
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
import { test, expect, type Page } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

let page: Page;

test.beforeAll(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto('/email-sign-up');
});

test.afterAll(async () => {
  await page.close();
});

test.describe('Email sign-up page', () => {

  test('The page renders with correct title', async () => {
    await expect(page).toHaveTitle(
        `Email Sign Up | freeCodeCamp.org`
      );
  });

  test('The page renders the correct content', async () => {
      await expect(
        page.locator(
          "text=freeCodeCamp is a proven path to your first software developer job."
        )
      ).toBeVisible();
});

  test('The sign up button renders and functions', async() => {
    const button = page.getByTestId('email-sign-up-button')

    await expect(button).toBeVisible()

    await button.click()

    await expect(page).toHaveURL(/.*\/learn\/?$/);

Review comment (suggestion):
```suggestion
test.beforeEach(async ({ page }) => {
  await page.goto('/email-sign-up');
});
```

We should ideally run the tests in isolation.

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const apiLocation = process.env.API_LOCATION || 'http://localhost:3000';

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();

    const signupLink = page.getByRole('link', {
      name: translations.buttons['sign-up-email-list']
    });
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
import { test, expect, type Page } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

let page: Page;

test.beforeAll(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto('/email-sign-up');
});

test.afterAll(async () => {
  await page.close();
});

test.describe('Email sign-up page', () => {

  test('The page renders with correct title', async () => {
    await expect(page).toHaveTitle(
        `Email Sign Up | freeCodeCamp.org`
      );
  });

  test('The page renders the correct content', async () => {
      await expect(
        page.locator(
          "text=freeCodeCamp is a proven path to your first software developer job."
        )
      ).toBeVisible();
});

Review comment (suggestion):
```suggestion
import { test, expect } from '@playwright/test';
```

Remove the unused translations import and global `page` variable.

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const apiLocation = process.env.API_LOCATION || 'http://localhost:3000';

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
Repository: freeCodeCamp/freeCodeCamp
File: e2e/email-sign-up.spec.ts (TypeScript)

Code before:
import { test, expect, type Page } from '@playwright/test';
import translations from '../client/i18n/locales/english/translations.json';

let page: Page;

test.beforeAll(async ({ browser }) => {
  page = await browser.newPage();
  await page.goto('/email-sign-up');
});

test.afterAll(async () => {
  await page.close();
});

test.describe('Email sign-up page', () => {

  test('The page renders with correct title', async () => {
    await expect(page).toHaveTitle(
        `Email Sign Up | freeCodeCamp.org`
      );
  });

  test('The page renders the correct content', async () => {
      await expect(
        page.locator(
          "text=freeCodeCamp is a proven path to your first software developer job."
        )
      ).toBeVisible();
});

  test('The sign up button renders and functions', async() => {
    const button = page.getByTestId('email-sign-up-button')

    await expect(button).toBeVisible()

    await button.click()

    await expect(page).toHaveURL(/.*\/learn\/?$/);
  })


});

Review comment (suggestion):
```suggestion
  test('The page renders with correct title', async ({ page }) => {
```

Because we can get `page` directly from the function args.

Code after:
import { test, expect } from '@playwright/test';

import translations from '../client/i18n/locales/english/translations.json';

const apiLocation = process.env.API_LOCATION || 'http://localhost:3000';

test.describe('Email sign-up page when user is not signed in', () => {
  test.use({ storageState: { cookies: [], origins: [] } });

  test.beforeEach(async ({ page }) => {
    // Intercept the endpoint to prevent `acceptedPrivacyTerms` from being set
    await page.route('*/**/update-privacy-terms', async route => {
      const json = [{ message: 'flash.privacy-updated', type: 'success' }];
      await route.fulfill({ json });
    });

    await page.goto('/email-sign-up');
  });

  test("should not enable Quincy's weekly newsletter when the user clicks the sign up button", async ({
    page,
    browserName
  }) => {
    test.skip(
      browserName === 'webkit',
      'user appears to not signed in on Webkit'
    );

    await expect(page).toHaveTitle('Email Sign Up | freeCodeCamp.org');
    await expect(
      page.getByText(
        'freeCodeCamp is a proven path to your first software developer job.'
      )
    ).toBeVisible();

    const signupLink = page.getByRole('link', {
      name: translations.buttons['sign-up-email-list']
    });

    await expect(signupLink).toBeVisible();
    await expect(signupLink).toHaveAttribute('href', `${apiLocation}/signin`);
    await signupLink.click();