ronantakizawa/codereview-bench huggingface dataset:
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({ directions: ['ltr', 'rtl'], modes: ['ios', '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`));
    });

    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>
            bottom

Review comment (suggestion):
```suggestion
configs({ directions: ['ltr'], modes: ['ios', 'md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
```

I think we can skip the rtl screenshots here since nothing on the basic divider changes

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(
        `
Repository: ionic-team/ionic-framework
File: core/src/components/divider/test/spacing/divider.e2e.ts (TypeScript)

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

configs({ directions: ['ltr', 'rtl'], modes: ['ios', 'md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('divider: spacing'), () => {
    ['xxsmall', 'xsmall', 'small', 'medium', 'large', 'xlarge', 'xxlarge'].forEach((spacing) => {
      test(`should render divider with ${spacing} spacing`, async ({ page }) => {
        await page.setContent(
          `
            <div id="container">
              top
              <ion-divider spacing="${spacing}"></ion-divider>
              bottom
            </div>
          `,
          config
        );

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

        await expect(container).toHaveScreenshot(screenshot(`divider-spacing-${spacing}`));
      });
    });
  });
});


Review comment (suggestion):
```suggestion
configs({ directions: ['ltr'], modes: ['ios', 'md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
```

We can skip rtl here since the spacing only applies to top/bottom

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: spacing'), () => {
    ['xxsmall', 'xsmall', 'small', 'medium', 'large', 'xlarge', 'xxlarge'].forEach((spacing) => {
      test(`should render divider with ${spacing} spacing`, async ({ page }) => {
        await page.setContent(
          `
            <div id="container">
              top
              <ion-divider spacing="${spacing}"></ion-divider>
              bottom
            </div>
          `,
          config
        );

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

        await expect(container).toHaveScreenshot(screenshot(`divider-spacing-${spacing}`));
      });
    });
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/divider/test/basic/divider.e2e.ts (TypeScript)

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

configs({ directions: ['ltr', 'rtl'], modes: ['ios', '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`));
    });

    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>
            bottom
          </div>
        `,
        config
      );

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

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


Review comment (suggestion):
If you want to keep the rtl tests for inset since it applies to left/right then you can move it to a separate config like this:

```ts
configs({ modes: ['ios', 'md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
```

If you leave `directions` out of the config it will run for both `ltr` and `rtl` 🙂 

Code after:
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>
            bottom
          </div>
        `,
        config
      );

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

      await expect(container).toHaveScreenshot(screenshot(`divider-basic-inset`));
    });
  });
});
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({ directions: ['ltr', 'rtl'], modes: ['ios', '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`));
    });

    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>
            bottom

Review comment (suggestion):
```suggestion
configs({ directions: ['ltr'], modes: ['md', 'ionic-md'] }).forEach(({ config, screenshot, title }) => {
```

Optional: we could actually skip `ios` for all of these tests since ios and md use the same values

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(
        `
Repository: ionic-team/ionic-framework
File: core/src/components/select/select.tsx (TypeScript)

Code before:
     * ion-alert when
     * using Custom Elements build.
     */
    // eslint-disable-next-line
    if (false) {
      // eslint-disable-next-line
      // @ts-ignore
      document.createElement('ion-alert');
    }

    return alertController.create(alertOpts);
  }

  private openModal() {
    const { multiple, value, interfaceOptions } = this;
    const mode = getIonMode(this);

    const modalOpts: ModalOptions = {
      ...interfaceOptions,
      mode,

      cssClass: ['select-modal', interfaceOptions.cssClass],
      component: 'ion-select-modal',
      componentProps: {
        header: interfaceOptions.header,
        closeText: interfaceOptions.closeText,
        multiple,
        value,
        options: this.createOverlaySelectOptions(this.childOpts, value),
      },
    };

    /**
     * Workaround for Stencil to autodefine
     * ion-select-modal and ion-modal when
     * using Custom Elements build.
     */
    // eslint-disable-next-line
    if (false) {
      // eslint-disable-next-line
      // @ts-ignore
      document.createElement('ion-select-modal');
      document.createElement('ion-modal');
    }

    return modalController.create(modalOpts);
  }

  /**
   * Close the select interface.
   */

Review comment (suggestion):
cancelText: this.cancelText,

I consider better to use the existing property cancelText, instead of creating a new one, so we avoid having two properties for the same thing.

Code after:
    if (false) {
      // eslint-disable-next-line
      // @ts-ignore
      document.createElement('ion-select-popover');
      document.createElement('ion-popover');
    }

    return popoverController.create(popoverOpts);
  }

  private async openActionSheet() {
    const mode = getIonMode(this);
    const interfaceOptions = this.interfaceOptions;
    const actionSheetOpts: ActionSheetOptions = {
      mode,
      ...interfaceOptions,

      buttons: this.createActionSheetButtons(this.childOpts, this.value),
      cssClass: ['select-action-sheet', interfaceOptions.cssClass],
    };

    /**
     * Workaround for Stencil to autodefine
     * ion-action-sheet when
     * using Custom Elements build.
     */
    // eslint-disable-next-line
    if (false) {
      // eslint-disable-next-line
      // @ts-ignore
      document.createElement('ion-action-sheet');
    }

    return actionSheetController.create(actionSheetOpts);
  }

  private async openAlert() {
    const interfaceOptions = this.interfaceOptions;
    const inputType = this.multiple ? 'checkbox' : 'radio';
    const mode = getIonMode(this);

    const alertOpts: AlertOptions = {
      mode,
      ...interfaceOptions,

      header: interfaceOptions.header ? interfaceOptions.header : this.labelText,
      inputs: this.createAlertInputs(this.childOpts, inputType, this.value),
      buttons: [
        {
          text: this.cancelText,
          role: 'cancel',
Repository: ionic-team/ionic-framework
File: core/src/components/avatar/test/basic/avatar.e2e.ts (TypeScript)

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

/**
 * Avatar does not test RTL behaviors.
 * Usages of Avatar in slots are tested in components that use Avatar.
 */
configs({ directions: ['ltr'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('avatar: basic'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto(`/src/components/avatar/test/basic`, config);

      const avatar = page.locator('#avatar');
      const avatarChip = page.locator('#avatar-chip');
      const avatarItemStart = page.locator('#avatar-item-start');
      const avatarItemEnd = page.locator('#avatar-item-end');
      const avatarCharactersDisabled = page.locator('#avatar-characters-disabled');
      const avatarDisabled = page.locator('#avatar-disabled');

      await expect(avatar).toHaveScreenshot(screenshot(`avatar-diff`));
      await expect(avatarChip).toHaveScreenshot(screenshot(`avatar-chip-diff`));
      await expect(avatarItemStart).toHaveScreenshot(screenshot(`avatar-item-start-diff`));
      await expect(avatarItemEnd).toHaveScreenshot(screenshot(`avatar-item-end-diff`));
      await expect(avatarCharactersDisabled).toHaveScreenshot(screenshot(`avatar-characters-disabled-diff`));
      await expect(avatarDisabled).toHaveScreenshot(screenshot(`avatar-disabled-diff`));
    });
  });
});


Review comment (suggestion):
Can we move these tests to only take screenshots for the `ionic` theme and also use `setContent` in order to capture just the avatar. Here is an example: https://github.com/ionic-team/ionic-framework/blob/40932d7b2d9f6650ed621e0183b9c70b74debff8/core/src/components/badge/test/shape/badge.e2e.ts#L7-L24

Please make sure not to include the `ion-item` in the screenshots.


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

/**
 * Avatar does not test RTL behaviors.
 * Usages of Avatar in slots are tested in components that use Avatar.
 */
configs({ directions: ['ltr'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('avatar: basic'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.goto(`/src/components/avatar/test/basic`, config);

      const avatar = page.locator('#avatar');
      const avatarChip = page.locator('#avatar-chip');
      const avatarItemStart = page.locator('#avatar-item-start');
      const avatarItemEnd = page.locator('#avatar-item-end');

      await expect(avatar).toHaveScreenshot(screenshot(`avatar-diff`));
      await expect(avatarChip).toHaveScreenshot(screenshot(`avatar-chip-diff`));
      await expect(avatarItemStart).toHaveScreenshot(screenshot(`avatar-item-start-diff`));
      await expect(avatarItemEnd).toHaveScreenshot(screenshot(`avatar-item-end-diff`));
    });
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/avatar/avatar.tsx (TypeScript)

Code before:
    // TODO(ROU-10755): Remove theme check when shapes are defined for all themes.
    if (theme !== 'ionic') {
      return undefined;
    }

    if (shape === undefined) {
      return 'round';
    }

    return shape;
  }

  render() {
    const theme = getIonTheme(this);
    const size = this.getSize();
    const shape = this.getShape();

    return (
      <Host
        class={{
          [theme]: true,
          [`avatar-${size}`]: size !== undefined,
          [`avatar-${shape}`]: shape !== undefined,
          [`avatar-image`]: this.hasImage,
          [`avatar-icon`]: this.hasIcon,
          [`avatar-disabled`]: this.disabled,
        }}
      >
        <slot></slot>
      </Host>
    );
  }
}


Review comment (suggestion):
Nit: Might be worth doing something like
```typescript
const { disabled } = this;
[...]
[`avatar-disabled`]: disabled
```
You could do that with `hasImage` and `hasIcon` too and things might look a little more clean. This is just a nit though, no worries if you don't want to do that.

Code after:
    // TODO(ROU-10755): Remove theme check when shapes are defined for all themes.
    if (theme !== 'ionic') {
      return undefined;
    }

    if (shape === undefined) {
      return 'round';
    }

    return shape;
  }

  render() {
    const { hasImage, hasIcon, disabled } = this;
    const theme = getIonTheme(this);
    const size = this.getSize();
    const shape = this.getShape();

    return (
      <Host
        class={{
          [theme]: true,
          [`avatar-${size}`]: size !== undefined,
          [`avatar-${shape}`]: shape !== undefined,
          [`avatar-image`]: hasImage,
          [`avatar-icon`]: hasIcon,
          [`avatar-disabled`]: disabled,
        }}
      >
        <slot></slot>
      </Host>
    );
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/avatar/test/states/avatar.e2e.ts (TypeScript)

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

/**
 * Avatar does not test RTL behaviors.
 * Usages of Avatar in slots are tested in components that use Avatar.
 */
configs({ directions: ['ltr'], modes: ['ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('avatar: states'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(
        ` 
        <div id="container"> 
          <ion-avatar id="avatar-characters-disabled" disabled> AV </ion-avatar>
          <ion-avatar id="avatar-characters-disabled" disabled>
            <img src="/src/components/avatar/test/avatar.svg" />
          </ion-avatar>
        </div> 
      `,
        config
      );
      const avatarCharactersDisabled = page.locator('#avatar-characters-disabled');
      const avatarDisabled = page.locator('#avatar-disabled');
      await expect(avatarCharactersDisabled).toHaveScreenshot(screenshot(`avatar-characters-disabled-diff`));
      await expect(avatarDisabled).toHaveScreenshot(screenshot(`avatar-disabled-diff`));
    });
  });
});


Review comment (suggestion):
```suggestion
      const container = page.locator('#container');

      await expect(container).toHaveScreenshot(screenshot(`avatar-disabled`));
```

Instead of querying for all of these we could just take one screenshot of the container 

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

/**
 * Avatar does not test RTL behaviors.
 * Usages of Avatar in slots are tested in components that use Avatar.
 */
configs({ directions: ['ltr'], modes: ['ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('avatar: states'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(
        ` 
        <div id="container"> 
          <ion-avatar disabled> AV </ion-avatar>
          <ion-avatar disabled>
            <img src="/src/components/avatar/test/avatar.svg" />
          </ion-avatar>
        </div> 
      `,
        config
      );
      const container = page.locator('#container');

      await expect(container).toHaveScreenshot(screenshot(`avatar-disabled`));
    });
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/avatar/test/states/avatar.e2e.ts (TypeScript)

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

/**
 * Avatar does not test RTL behaviors.
 * Usages of Avatar in slots are tested in components that use Avatar.
 */
configs({ directions: ['ltr'], modes: ['ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('avatar: states'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(
        ` 
        <div id="container"> 
          <ion-avatar id="avatar-characters-disabled" disabled> AV </ion-avatar>
          <ion-avatar id="avatar-characters-disabled" disabled>
            <img src="/src/components/avatar/test/avatar.svg" />
          </ion-avatar>
        </div> 
      `,
        config
      );
      const container = page.locator('#container');

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


Review comment (suggestion):
```suggestion
          <ion-avatar disabled> AV </ion-avatar>
          <ion-avatar disabled>
```

You can remove the ids here too but shouldn't need to generate screenshots again after

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

/**
 * Avatar does not test RTL behaviors.
 * Usages of Avatar in slots are tested in components that use Avatar.
 */
configs({ directions: ['ltr'], modes: ['ionic-md'] }).forEach(({ config, screenshot, title }) => {
  test.describe(title('avatar: states'), () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(
        ` 
        <div id="container"> 
          <ion-avatar disabled> AV </ion-avatar>
          <ion-avatar disabled>
            <img src="/src/components/avatar/test/avatar.svg" />
          </ion-avatar>
        </div> 
      `,
        config
      );
      const container = page.locator('#container');

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