ronantakizawa/codereview-bench huggingface dataset:
Repository: ionic-team/ionic-framework
File: core/src/components/button/test/spinner/button.e2e.ts (TypeScript)

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

configs({ modes: ['ionic-md', 'ionic-ios'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('button: spinner'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto(`/src/components/button/test/spinner`, config);

      await page.setIonViewport();

      await expect(page).toHaveScreenshot(screenshot(`button-spinner`));
    });
  });
});


Review comment (suggestion):
Since the functionality doesn't change, we don't need to test for all the modes. I also realize that we don't need to test RTL since functionality doesn't change.

I suggest only testing `ionic-md`. Make sure to remove the snapshots for `ionic-ios` and RTL.

```suggestion
configs({ modes: ['ionic-md'], directions: ['ltr'] }).forEach(({ config, screenshot, title }) => {
```

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

configs({ modes: ['ionic-md'], directions: ['ltr'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('button: spinner'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto(`/src/components/button/test/spinner`, config);

      await page.setIonViewport();

      await expect(page).toHaveScreenshot(screenshot(`button-spinner`));
    });
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/badge/test/hint/badge.e2e.ts (TypeScript)

Code before:
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#avatar');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-avatar`));
    });
  });

  test.describe(title('badge: hint inside tab button'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button`));
    });
  });

  test.describe(title('badge: hint inside tab button when tab button has icon at bottom'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button-icon-bottom');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-bottom`));
    });
  });
});


Review comment (suggestion):
We can group these together under the same describe. Notice that I changed `badge-hint-tab-button` to be `badge-hint-tab-button-icon-top` so you'll have to delete the `badge-hint-tab-button` snapshots.

```suggestion
   test('should not have visual regressions when ion-icon at the top', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-top`));
    });

    test('should not have visual regressions when ion-icon at the bottom', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button-icon-bottom');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-bottom`));
    });
```

Code after:
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#avatar');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-avatar`));
    });
  });

  test.describe(title('badge: hint inside tab button'), () => {
    test('should not have visual regressions when icon is on the top', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-top`));
    });

    test('should not have visual regressions when icon is at the bottom', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button-icon-bottom');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-bottom`));
    });
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/badge/test/hint/badge.e2e.ts (TypeScript)

Code before:
      const container = page.locator('#empty');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-empty`));
    });
  });

  test.describe(title('badge: hint inside avatar'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#avatar');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-avatar`));
    });
  });

  test.describe(title('badge: hint inside tab button'), () => {
    test('tab button icon top, should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-top`));
    });

    test('tab button icon bottom, should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button-icon-bottom');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-bottom`));
    });
  });
});


Review comment (suggestion):
```suggestion
    test('should not have visual regressions when icon is at the bottom', async ({ page }) => {
```
This follows the title standards for testing.

Code after:
      const container = page.locator('#empty');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-empty`));
    });
  });

  test.describe(title('badge: hint inside avatar'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#avatar');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-avatar`));
    });
  });

  test.describe(title('badge: hint inside tab button'), () => {
    test('should not have visual regressions when icon is on the top', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-top`));
    });

    test('should not have visual regressions when icon is at the bottom', async ({ page }) => {
      await page.goto('/src/components/badge/test/hint', config);

      const container = page.locator('#tab-button-icon-bottom');

      await expect(container).toHaveScreenshot(screenshot(`badge-hint-tab-button-icon-bottom`));
    });
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/divider/divider.tsx (TypeScript)

Code before:
import { getIonTheme } from '@global/ionic-global';
import type { ComponentInterface } from '@stencil/core';
import { Component, Prop, Host, h } from '@stencil/core';

@Component({
  tag: 'ion-divider',
  styleUrls: {
    ios: 'divider.scss',
    md: 'divider.scss',
    ionic: 'divider.scss',
  },
  shadow: true,
})
export class Divider implements ComponentInterface {
  /**
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for a very small spacing.
   * Set to `"small"` for a small spacing.
   * Set to "medium" for a medium spacing.
   * Set to "large" for a large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"small"` for the `ionic` theme, undefined for all other themes.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset?: boolean = false;

  private getSpacing(): string | undefined {
    const { spacing } = this;

    if (spacing === undefined) {
      return 'xxsmall';
    }

    return spacing;
  }

  render() {
    const theme = getIonTheme(this);
    const spacing = this.getSpacing();
    return (
      <Host
        class={{
          [theme]: true,
          [`divider-space-${spacing}`]: spacing !== undefined,

Review comment (suggestion):
```suggestion
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' = 'xxsmall';
```

We can set the default here

Code after:
import { getIonTheme } from '@global/ionic-global';
import type { ComponentInterface } from '@stencil/core';
import { Component, Prop, Host, h } from '@stencil/core';

@Component({
  tag: 'ion-divider',
  styleUrls: {
    ios: 'divider.ios.scss',
    md: 'divider.md.scss',
    ionic: 'divider.ionic.scss',
  },
  shadow: true,
})
export class Divider implements ComponentInterface {
  /**
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for very small spacing.
   * Set to `"small"` for small spacing.
   * Set to "medium" for medium spacing.
   * Set to "large" for large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"xxsmall"`.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' =
    'xxsmall';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset: boolean = false;

  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);

    return (
      <Host
        class={{
          [theme]: true,
          [`divider-spacing-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: inset,
        }}
      >
        <hr />
      </Host>
    );
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/divider/divider.tsx (TypeScript)

Code before:
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for a very small spacing.
   * Set to `"small"` for a small spacing.
   * Set to "medium" for a medium spacing.
   * Set to "large" for a large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"small"` for the `ionic` theme, undefined for all other themes.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset?: boolean = false;

  private getSpacing(): string | undefined {
    const { spacing } = this;

    if (spacing === undefined) {
      return 'xxsmall';
    }

    return spacing;
  }

  render() {
    const theme = getIonTheme(this);
    const spacing = this.getSpacing();
    return (
      <Host
        class={{
          [theme]: true,
          [`divider-space-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: this.inset || false,
        }}
      >
        <hr />
      </Host>
    );
  }
}


Review comment (suggestion):
```suggestion
```

We don't need this function since we can set the value in the property.

Code after:
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for very small spacing.
   * Set to `"small"` for small spacing.
   * Set to "medium" for medium spacing.
   * Set to "large" for large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"xxsmall"`.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' =
    'xxsmall';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset: boolean = false;

  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);

    return (
      <Host
        class={{
          [theme]: true,
          [`divider-spacing-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: inset,
        }}
      >
        <hr />
      </Host>
    );
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/divider/divider.tsx (TypeScript)

Code before:
})
export class Divider implements ComponentInterface {
  /**
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for a very small spacing.
   * Set to `"small"` for a small spacing.
   * Set to "medium" for a medium spacing.
   * Set to "large" for a large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"small"` for the `ionic` theme, undefined for all other themes.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset?: boolean = false;

  private getSpacing(): string | undefined {
    const { spacing } = this;

    if (spacing === undefined) {
      return 'xxsmall';
    }

    return spacing;
  }

  render() {
    const theme = getIonTheme(this);
    const spacing = this.getSpacing();
    return (
      <Host
        class={{
          [theme]: true,
          [`divider-space-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: this.inset || false,
        }}
      >
        <hr />
      </Host>
    );
  }
}


Review comment (suggestion):
```suggestion
  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);
    
    return (
```

Code after:
})
export class Divider implements ComponentInterface {
  /**
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for very small spacing.
   * Set to `"small"` for small spacing.
   * Set to "medium" for medium spacing.
   * Set to "large" for large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"xxsmall"`.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' =
    'xxsmall';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset: boolean = false;

  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);

    return (
      <Host
        class={{
          [theme]: true,
          [`divider-spacing-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: inset,
        }}
      >
        <hr />
      </Host>
    );
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/divider/divider.tsx (TypeScript)

Code before:
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset?: boolean = false;

  private getSpacing(): string | undefined {
    const { spacing } = this;

    if (spacing === undefined) {
      return 'xxsmall';
    }

    return spacing;
  }

  render() {
    const theme = getIonTheme(this);
    const spacing = this.getSpacing();
    return (
      <Host
        class={{
          [theme]: true,
          [`divider-space-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: this.inset || false,
        }}
      >
        <hr />
      </Host>
    );
  }
}


Review comment (suggestion):
```suggestion
          [`divider-spacing-${spacing}`]: spacing !== undefined,
```

Let's rename this class to match the property name (also update in the scss file)

Code after:
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' =
    'xxsmall';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset: boolean = false;

  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);

    return (
      <Host
        class={{
          [theme]: true,
          [`divider-spacing-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: inset,
        }}
      >
        <hr />
      </Host>
    );
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/divider/divider.tsx (TypeScript)

Code before:

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset?: boolean = false;

  private getSpacing(): string | undefined {
    const { spacing } = this;

    if (spacing === undefined) {
      return 'xxsmall';
    }

    return spacing;
  }

  render() {
    const theme = getIonTheme(this);
    const spacing = this.getSpacing();
    return (
      <Host
        class={{
          [theme]: true,
          [`divider-space-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: this.inset || false,
        }}
      >
        <hr />
      </Host>
    );
  }
}


Review comment (suggestion):
```suggestion
          [`divider-inset`]: inset,
```

Code after:
    'xxsmall';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset: boolean = false;

  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);

    return (
      <Host
        class={{
          [theme]: true,
          [`divider-spacing-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: inset,
        }}
      >
        <hr />
      </Host>
    );
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/divider/divider.tsx (TypeScript)

Code before:
import { getIonTheme } from '@global/ionic-global';
import type { ComponentInterface } from '@stencil/core';
import { Component, Prop, Host, h } from '@stencil/core';

@Component({
  tag: 'ion-divider',
  styleUrls: {
    ios: 'divider.scss',
    md: 'divider.scss',
    ionic: 'divider.scss',
  },
  shadow: true,
})
export class Divider implements ComponentInterface {
  /**
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for a very small spacing.
   * Set to `"small"` for a small spacing.
   * Set to "medium" for a medium spacing.
   * Set to "large" for a large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"small"` for the `ionic` theme, undefined for all other themes.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset?: boolean = false;

  private getSpacing(): string | undefined {
    const { spacing } = this;

    if (spacing === undefined) {
      return 'xxsmall';
    }

    return spacing;
  }

  render() {
    const theme = getIonTheme(this);
    const spacing = this.getSpacing();
    return (

Review comment (suggestion):
```suggestion
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for very small spacing.
   * Set to `"small"` for small spacing.
   * Set to "medium" for medium spacing.
   * Set to "large" for large spacing.
   * Set to `"xlarge"` for the largest spacing.
```

Code after:
import { getIonTheme } from '@global/ionic-global';
import type { ComponentInterface } from '@stencil/core';
import { Component, Prop, Host, h } from '@stencil/core';

@Component({
  tag: 'ion-divider',
  styleUrls: {
    ios: 'divider.ios.scss',
    md: 'divider.md.scss',
    ionic: 'divider.ionic.scss',
  },
  shadow: true,
})
export class Divider implements ComponentInterface {
  /**
   * Set to `"xxsmall"` for the smallest spacing.
   * Set to "xsmall" for very small spacing.
   * Set to `"small"` for small spacing.
   * Set to "medium" for medium spacing.
   * Set to "large" for large spacing.
   * Set to `"xlarge"` for the largest spacing.
   *
   * Defaults to `"xxsmall"`.
   */
  @Prop({ reflect: true }) spacing?: 'xxsmall' | 'xsmall' | 'small' | 'medium' | 'large' | 'xlarge' | 'xxlarge' =
    'xxsmall';

  /**
   * If `true`, the divider will have horizontal margins
   * By default, it's `false`
   */
  @Prop() inset: boolean = false;

  render() {
    const { inset, spacing } = this;
    const theme = getIonTheme(this);

    return (
      <Host
        class={{
          [theme]: true,
          [`divider-spacing-${spacing}`]: spacing !== undefined,
          [`divider-inset`]: inset,
        }}
      >
        <hr />
Repository: ionic-team/ionic-framework
File: core/src/components/divider/test/basic/divider.e2e.ts (TypeScript)

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

configs().forEach(({ config, screenshot, title }) => {
  test.describe(title('divider: rendering'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto('/src/components/divider/test/basic', config);

      await page.setIonViewport();

      await expect(page).toHaveScreenshot(screenshot(`divider-basic`));
    });
  });
});


Review comment (suggestion):
Instead of taking a screenshot of the entire page, we should use `setContent` to take screenshots of just the divider in different scenarios. You can find an example here: https://github.com/ionic-team/ionic-framework/blob/40932d7b2d9f6650ed621e0183b9c70b74debff8/core/src/components/avatar/test/shape/avatar.e2e.ts#L11-L16



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

configs({ directions: ['ltr'], modes: ['md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('divider: rendering'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(
        `
          <div id="container">
            top
            <ion-divider></ion-divider>
            bottom
          </div>
        `,
        config
      );

      const container = page.locator('#container');

      await expect(container).toHaveScreenshot(screenshot(`divider-basic-default`));
    });
  });
});

configs({ modes: ['md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('divider: inset'), () => {
    test('should not have visual regressions when inset is enabled', async ({ page }) => {
      await page.setContent(
        `
          <div id="container">
            top
            <ion-divider inset="true"></ion-divider>