ronantakizawa/codereview-bench huggingface dataset:
Repository: ionic-team/ionic-framework
File: core/src/components/datetime/datetime.tsx (TypeScript)

Code before:
    if (popoverOrModal) {
      popoverOrModal.dismiss();
    }
  };

  private setWorkingParts = (parts: DatetimeParts) => {
    this.workingParts = {
      ...parts,
    };
  };

  private setActiveParts = (parts: DatetimeParts) => {
    this.activeParts = {
      ...parts,
    };

    const hasSlottedButtons = this.el.querySelector('[slot="buttons"]') !== null;
    if (hasSlottedButtons || this.showDefaultButtons) {
      return;
    }

    this.confirm();
  };

  private get isCalendarPicker() {
    return this.presentation === 'date' || this.presentation === 'date-time' || this.presentation === 'time-date';
  }

  /**
   * Stencil sometimes sets calendarBodyRef to null on rerender, even though
   * the element is present. Query for it manually as a fallback.
   *
   * TODO(FW-901) Remove when issue is resolved: https://github.com/ionic-team/stencil/issues/3253
   */
  private getCalendarBodyEl = () => {
    return this.calendarBodyRef || this.el.shadowRoot?.querySelector('.calendar-body');
  };

  private initializeKeyboardListeners = () => {
    const calendarBodyRef = this.getCalendarBodyEl();
    if (!calendarBodyRef) {
      return;
    }

    const root = this.el!.shadowRoot!;

    /**
     * Get a reference to the month
     * element we are currently viewing.
     */
    const currentMonth = calendarBodyRef.querySelector('.calendar-month:nth-of-type(2)')!;

Review comment (suggestion):
```suggestion
    const { presentation } = this;
    return presentation === 'date' || presentation === 'date-time' || presentation === 'time-date';
```

Code after:
    if (popoverOrModal) {
      popoverOrModal.dismiss();
    }
  };

  private setWorkingParts = (parts: DatetimeParts) => {
    this.workingParts = {
      ...parts,
    };
  };

  private setActiveParts = (parts: DatetimeParts) => {
    this.activeParts = {
      ...parts,
    };

    const hasSlottedButtons = this.el.querySelector('[slot="buttons"]') !== null;
    if (hasSlottedButtons || this.showDefaultButtons) {
      return;
    }

    this.confirm();
  };

  private get isCalendarPicker() {
    const { presentation } = this;
    return presentation === 'date' || presentation === 'date-time' || presentation === 'time-date';
  }

  /**
   * Stencil sometimes sets calendarBodyRef to null on rerender, even though
   * the element is present. Query for it manually as a fallback.
   *
   * TODO(FW-901) Remove when issue is resolved: https://github.com/ionic-team/stencil/issues/3253
   */
  private getCalendarBodyEl = () => {
    return this.calendarBodyRef || this.el.shadowRoot?.querySelector('.calendar-body');
  };

  private initializeKeyboardListeners = () => {
    const calendarBodyRef = this.getCalendarBodyEl();
    if (!calendarBodyRef) {
      return;
    }

    const root = this.el!.shadowRoot!;

    /**
     * Get a reference to the month
     * element we are currently viewing.
     */
Repository: ionic-team/ionic-framework
File: core/src/components/chip/test/states/chip.e2e.ts (TypeScript)

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

test.describe('chip: rendering', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/chip/test/states');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`chip-states-${page.getSnapshotSettings()}.png`);
  });
});


Review comment (suggestion):
```suggestion
test.describe('chip: states', () => {
```

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

test.describe('chip: states', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/chip/test/states');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`chip-states-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/chip/test/standalone/chip.e2e.ts (TypeScript)

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

test.describe('chip: rendering', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/chip/test/standalone');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`chip-standalone-${page.getSnapshotSettings()}.png`);
  });
});


Review comment (suggestion):
```suggestion
test.describe('chip: standalone', () => {
```

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

test.describe('chip: standalone', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/chip/test/standalone');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`chip-standalone-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/skeleton-text/test/basic/skeleton-text.e2e.ts (TypeScript)

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

test.describe('skeleton-text: rendering', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/skeleton-text/test/basic');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`skeleton-text-basic-${page.getSnapshotSettings()}.png`);
  });
});


Review comment (suggestion):
```suggestion
test.describe('skeleton-text: basic', () => {
```
```suggestion
test.describe('skeleton-text: rendering', () => {
```

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

test.describe('skeleton-text: basic', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/skeleton-text/test/basic');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`skeleton-text-basic-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/skeleton-text/test/custom/skeleton-text.e2e.ts (TypeScript)

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

test.describe('skeleton-text: rendering', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/skeleton-text/test/custom');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`skeleton-text-custom-${page.getSnapshotSettings()}.png`);
  });
});


Review comment (suggestion):
```suggestion
test.describe('skeleton-text: custom', () => {
```
```suggestion
test.describe('skeleton-text: rendering', () => {
```

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

test.describe('skeleton-text: custom', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/skeleton-text/test/custom');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`skeleton-text-custom-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/img/test/basic/img.e2e.ts (TypeScript)

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

test.describe('img: basic', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/img/test/basic');

    const ionImgDidLoad = await page.spyOnEvent('ionImgDidLoad');

    await page.setIonViewport();

    await ionImgDidLoad.next();

    expect(await page.screenshot()).toMatchSnapshot(`img-basic-${page.getSnapshotSettings()}.png`);
  });
});


Review comment (refactor):
There are multiple ion-img elements on this page, and this would only wait for the first occurrence of the event. It might be better to simplify the E2E test to only have on `ion-img` (or you could wait for each of the events to fire).

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

test.describe('img: basic', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto('/src/components/img/test/basic');

    /**
     * The first `ion-img` resolves before the spy can be created,
     * since the image is initially visible and the IO fires.
     *
     * We manually look at the image's completion state to determine
     * that it loaded, before creating a spy for the second image.
     */
    const img = await page.locator('.img-part img');
    await img.evaluate((el: HTMLImageElement) => el.complete);

    const ionImgDidLoad = await page.spyOnEvent('ionImgDidLoad');
    /**
     * Resizing the viewport will cause the intersection observers of
     * the remaining images to fire, which will cause the `ionImgDidLoad`
     * event to fire.
     */
    await page.setIonViewport();

    await ionImgDidLoad.next();

    expect(await page.screenshot()).toMatchSnapshot(`img-basic-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/textarea/test/a11y/textarea.e2e.ts (TypeScript)

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

test.describe('textarea: a11y', () => {
  test('does not set a default aria-labelledby when there is not a neighboring ion-label', async ({ page }) => {
    await page.setContent(`<ion-textarea></ion-textarea>`);

    await page.setIonViewport();

    const textarea = page.locator('ion-textarea textarea');
    const ariaLabelledBy = await textarea.getAttribute('aria-labelledby');

    expect(ariaLabelledBy).toBe(null);
  });

  test('set a default aria-labelledby when a neighboring ion-label exist', async ({ page }) => {
    await page.setContent(
      `<ion-item>
      <ion-label>A11y Test</ion-label>
      <ion-textarea></ion-textarea>
    </ion-item>`
    );

    const label = page.locator('ion-label');
    const textarea = page.locator('ion-textarea textarea');
    const ariaLabelledBy = await textarea.getAttribute('aria-labelledby');

    expect(ariaLabelledBy).toBe(await label.getAttribute('id'));
  });
});


Review comment (suggestion):
Optional

```suggestion
    await page.setContent(`
      <ion-item>
        <ion-label>A11y Test</ion-label>
        <ion-textarea></ion-textarea>
      </ion-item>
    `);
```

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

test.describe('textarea: a11y', () => {
  test('does not set a default aria-labelledby when there is not a neighboring ion-label', async ({ page }) => {
    await page.setContent(`<ion-textarea></ion-textarea>`);

    await page.setIonViewport();

    const textarea = page.locator('ion-textarea textarea');
    const ariaLabelledBy = await textarea.getAttribute('aria-labelledby');

    expect(ariaLabelledBy).toBe(null);
  });

  test('set a default aria-labelledby when a neighboring ion-label exist', async ({ page }) => {
    await page.setContent(`
      <ion-item>
        <ion-label>A11y Test</ion-label>
        <ion-textarea></ion-textarea>
      </ion-item>
    `);

    const label = page.locator('ion-label');
    const textarea = page.locator('ion-textarea textarea');
    const ariaLabelledBy = await textarea.getAttribute('aria-labelledby');

    expect(ariaLabelledBy).toBe(await label.getAttribute('id'));
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/textarea/test/autogrow/textarea.e2e.ts (TypeScript)

Code before:
    expect(await page.screenshot()).toMatchSnapshot(`textarea-autogrow-diff-${page.getSnapshotSettings()}.png`);
  });

  test('should grow when typing', async ({ page }) => {
    await page.setContent(
      `
      <ion-app>
        <ion-content>
          <ion-list>
            <ion-item>
              <ion-textarea auto-grow="true"></ion-textarea>
            </ion-item>
          </ion-list>
        </ion-content>
      </ion-app>`
    );

    const textarea = await page.waitForSelector('ion-textarea');

    await textarea.click();

    await textarea.type('Now, this is a story all about how');

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`textarea-autogrow-initial-${page.getSnapshotSettings()}.png`);

    await textarea.type(
      [
        `\nMy life got flipped-turned upside down`,
        `And I'd like to take a minute`,
        `Just sit right there`,
        `I'll tell you how I became the prince of a town called Bel-Air`,
      ].join('\n')
    );

    expect(await page.screenshot()).toMatchSnapshot(`textarea-autogrow-after-${page.getSnapshotSettings()}.png`);
  });
});


Review comment (suggestion):
~ Would it make sense to just do `textarea.screenshot` instead of the whole `page`, to reduce the size of the images? (For L46 as well)

Code after:
    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`textarea-autogrow-diff-${page.getSnapshotSettings()}.png`);
  });

  test('should grow when typing', async ({ page }) => {
    await page.setContent(
      `
      <ion-app>
        <ion-content>
          <ion-list>
            <ion-item>
              <ion-textarea auto-grow="true"></ion-textarea>
            </ion-item>
          </ion-list>
        </ion-content>
      </ion-app>`
    );

    const textarea = await page.waitForSelector('ion-textarea');

    await textarea.click();

    await textarea.type('Now, this is a story all about how');

    await page.setIonViewport();

    expect(await textarea.screenshot()).toMatchSnapshot(`textarea-autogrow-initial-${page.getSnapshotSettings()}.png`);

    await textarea.type(
      [
        `\nMy life got flipped-turned upside down`,
        `And I'd like to take a minute`,
        `Just sit right there`,
        `I'll tell you how I became the prince of a town called Bel-Air`,
      ].join('\n')
    );

    expect(await textarea.screenshot()).toMatchSnapshot(`textarea-autogrow-after-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/modal/gestures/sheet.ts (TypeScript)

Code before:
                  /**
                   * Backdrop should become enabled
                   * after the backdropBreakpoint value
                   */
                  const shouldEnableBackdrop = currentBreakpoint > backdropBreakpoint;
                  if (shouldEnableBackdrop) {
                    enableBackdrop();
                  } else {
                    disableBackdrop();
                  }

                  gesture.enable(true);
                });
              } else {
                gesture.enable(true);
              }
              resolve();
            }

            /**
             * This must be a one time callback
             * otherwise a new callback will
             * be added every time onEnd runs.
             */
          },
          { oneTimeCallback: true }
        )
        .progressEnd(1, 0, 500);

      if (shouldPreventDismiss) {
        handleCanDismiss(baseEl, animation);
      } else if (!shouldRemainOpen) {
        onDismiss();
      }
    });
  };

  const gesture = createGesture({
    el: wrapperEl,
    gestureName: 'modalSheet',
    gesturePriority: 40,
    direction: 'y',
    threshold: 10,
    canStart,
    onStart,
    onMove,
    onEnd,
  });

  return {
    gesture,

Review comment (suggestion):
~ Since this `resolve` is outside the `raf` called on L310, it could be hit before all the logic in there is done. I'm admittedly not sure of the practical implications there, but it may be worth calling it at the end of the `raf`, and after L339 to cover that case as well.

Code after:
                   * enable scrolling again.
                   */
                  if (contentEl && currentBreakpoint === breakpoints[breakpoints.length - 1]) {
                    contentEl.scrollY = true;
                  }

                  /**
                   * Backdrop should become enabled
                   * after the backdropBreakpoint value
                   */
                  const shouldEnableBackdrop = currentBreakpoint > backdropBreakpoint;
                  if (shouldEnableBackdrop) {
                    enableBackdrop();
                  } else {
                    disableBackdrop();
                  }

                  gesture.enable(true);
                  resolve();
                });
              } else {
                gesture.enable(true);
                resolve();
              }
            } else {
              resolve();
            }

            /**
             * This must be a one time callback
             * otherwise a new callback will
             * be added every time onEnd runs.
             */
          },
          { oneTimeCallback: true }
        )
        .progressEnd(1, 0, 500);
    });
  };

  const gesture = createGesture({
    el: wrapperEl,
    gestureName: 'modalSheet',
    gesturePriority: 40,
    direction: 'y',
    threshold: 10,
    canStart,
    onStart,
    onMove,
    onEnd,
  });
Repository: ionic-team/ionic-framework
File: core/src/components/input/test/basic/input.e2e.ts (TypeScript)

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

test.describe('input: basic', () => {
  test.beforeEach(async ({ page }) => {
    await page.goto('/src/components/input/test/basic');
  });

  test('should not have visual regressions', async ({ page }) => {
    const compares = [];

    await page.setIonViewport();

    compares.push({
      name: 'initial',
      screenshot: await page.screenshot(),
    });

    await page.click('#fullInput');

    const fullItem = page.locator('#fullItem');
    await expect(fullItem).toHaveClass(/item-has-focus/);

    compares.push({
      name: 'full input focused',
      screenshot: await page.screenshot(),
    });

    await page.click('#insetInput');

    const insetItem = page.locator('#insetItem');
    await expect(insetItem).toHaveClass(/item-has-focus/);

    compares.push({

Review comment (suggestion):
Thoughts on having one or more of these screenshots be just the `ion-item` containing the input in question, to cut down on screenshot size? This test doesn't have a general-purpose screenshot of the whole page, so I figure you could either add one and make all three just the items, or make only two of the items and leave the third to cover the whole page.

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

test.describe('input: basic', () => {
  test.describe('input with overflow', () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(`
        <ion-content>
          <ion-list>
            <ion-item>
              <ion-input value="reallylonglonglonginputtoseetheedgesreallylonglonglonginputtoseetheedges"></ion-input>
            </ion-item>
          </ion-list>
        </ion-content>
      `);
      const item = page.locator('ion-item');
      // Validates the display of an input where text extends off the edge of the component.
      expect(await item.screenshot()).toMatchSnapshot(`input-with-text-overflow-${page.getSnapshotSettings()}.png`);
    });
  });

  test.describe('input with placeholder', () => {
    test('should not have visual regressions', async ({ page }) => {
      await page.setContent(`
      <ion-content>
        <ion-list>
          <ion-item>
            <ion-input placeholder="Placeholder"></ion-input>
          </ion-item>
        </ion-list>
      </ion-content>
      `);
      const item = page.locator('ion-item');
      // Validates the display of an input with a placeholder.
Repository: ionic-team/ionic-framework
File: core/src/components/input/test/masking/input.e2e.ts (TypeScript)

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

test.describe('input: masking', () => {
  test.beforeEach(async ({ page }) => {
    await page.goto('/src/components/input/test/masking');
  });

  test('should filter out spaces', async ({ page }, testInfo) => {
    test.skip(testInfo.project.metadata.rtl === true, 'Does not test LTR vs. RTL layout.');

    const input = page.locator('#inputTrimmed');

    await input.click();

    await page.keyboard.type('S p a c e s');

    const currentValue = await input.evaluate((el: HTMLInputElement) => {
      return el.value;
    });

    await expect(currentValue).toEqual('Spaces');
  });
});


Review comment (suggestion):
You can probably just use `expect(...).toHaveJSProperty()` instead

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

test.describe('input: masking', () => {
  test.beforeEach(async ({ page }) => {
    await page.goto('/src/components/input/test/masking');
  });

  test('should filter out spaces', async ({ page }, testInfo) => {
    test.skip(testInfo.project.metadata.rtl === true, 'Does not test LTR vs. RTL layout.');

    const input = page.locator('#inputTrimmed');

    await input.click();

    await page.keyboard.type('S p a c e s');

    await expect(input).toHaveJSProperty('value', 'Spaces');
  });
});
Repository: ionic-team/ionic-framework
File: packages/react/test-app/src/pages/navigation/NavComponent.tsx (TypeScript)

Code before:
};

const PageThree = () => {
  return (
    <>
      <IonHeader>
        <IonToolbar>
          <IonTitle>Page Three</IonTitle>
          <IonButtons>
            <IonBackButton />
          </IonButtons>
        </IonToolbar>
      </IonHeader>
      <IonContent>
        <IonLabel>Page three content</IonLabel>
      </IonContent>
    </>
  );
};

const NavComponent: React.FC = () => {
  return (
    <IonPage>
      <IonNav
        root={() => <PageOne someString="Hello" someNumber={3} someBoolean={true} />}
        rootParams={{}}
      />
    </IonPage>
  );
};

export default NavComponent;


Review comment (refactor):
~ We can optionally remove this line. It was added to display the dev warning in the console, but isn't required as part of the test.

Code after:
          <IonButtons>
            <IonBackButton />
          </IonButtons>
        </IonToolbar>
      </IonHeader>
      <IonContent id="pageTwoContent">
        <IonLabel>Page two content</IonLabel>
        <div id="pageTwoProps">{JSON.stringify(props)}</div>
        <IonNavLink routerDirection="forward" component={PageThree}>
          <IonButton>Go to Page Three</IonButton>
        </IonNavLink>
      </IonContent>
    </>
  );
};

const PageThree = () => {
  return (
    <>
      <IonHeader>
        <IonToolbar>
          <IonTitle>Page Three</IonTitle>
          <IonButtons>
            <IonBackButton />
          </IonButtons>
        </IonToolbar>
      </IonHeader>
      <IonContent>
        <IonLabel>Page three content</IonLabel>
      </IonContent>
    </>
  );
};

const NavComponent: React.FC = () => {
  const ref = useRef<any>();
  return (
    <IonPage>
      <IonNav
        ref={ref}
        root={PageOne}
        rootParams={{
          someString: 'Hello',
          someNumber: 3,
          someBoolean: true,
          nav: ref,
        }}
      />
    </IonPage>
  );
};