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

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

test.describe('datetime: selecting a day', () => {
  const testHighlight = async (page: E2EPage, datetimeID: string) => {
    const today = new Date();
    await page.goto('/src/components/datetime/test/basic');

    const todayBtn = page.locator(
      `#${datetimeID} .calendar-day[data-day='${today.getDate()}'][data-month='${today.getMonth() + 1}']`
    );

    const classes = await todayBtn.evaluate((el: any) => el.classList.value);
    expect(classes).toMatch('calendar-day-today');
    expect(classes).not.toMatch('calendar-day-active');

    await todayBtn.click();
    await page.waitForChanges();

    const classes2 = await todayBtn.evaluate((el: any) => el.classList.value);
    expect(classes2).toMatch('calendar-day-active');
  };

  test('should not highlight a day until one is selected', async ({ page }) => {
    await testHighlight(page, 'inline-datetime-no-value');
  });

  test('should not highlight a day until one is selected, with default-buttons', async ({ page }) => {
    await testHighlight(page, 'custom-datetime');
  });
});

Review comment (suggestion):
You should be able to use the `toHaveClass` method instead: https://playwright.dev/docs/test-assertions#locator-assertions-to-have-class

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

test.describe('datetime: selecting a day', () => {
  const testHighlight = async (page: E2EPage, datetimeID: string) => {
    const today = new Date();
    await page.goto('/src/components/datetime/test/basic');

    const todayBtn = page.locator(
      `#${datetimeID} .calendar-day[data-day='${today.getDate()}'][data-month='${today.getMonth() + 1}']`
    );

    expect(todayBtn).toHaveClass(/calendar-day-today/);
    expect(todayBtn).not.toHaveClass(/calendar-day-active/);

    await todayBtn.click();
    await page.waitForChanges();

    expect(todayBtn).toHaveClass(/calendar-day-active/);
  };

  test('should not highlight a day until one is selected', async ({ page }) => {
    await testHighlight(page, 'inline-datetime-no-value');
  });

  test('should not highlight a day until one is selected, with default-buttons', async ({ page }) => {
    await testHighlight(page, 'custom-datetime');
  });
});
Repository: ionic-team/ionic-framework
File: core/src/utils/helpers.ts (TypeScript)

Code before:
  });

  return attributeObject;
};

const ariaAttributes = [
  'role',
  'aria-activedescendant',
  'aria-atomic',
  'aria-autocomplete',
  'aria-braillelabel',
  'aria-brailleroledescription',
  'aria-busy',
  'aria-checked',
  'aria-colcount',
  'aria-colindex',
  'aria-colindextext',
  'aria-colspan',
  'aria-controls',
  'aria-current',
  'aria-describedby',
  'aria-description',
  'aria-details',
  'aria-disabled',
  // @deprecated
  'aria-dropeffect',
  'aria-errormessage',
  'aria-expanded',
  'aria-flowto',
  // @deprecated
  'aria-grabbed',
  'aria-haspopup',
  'aria-hidden',
  'aria-invalid',
  'aria-keyshortcuts',
  'aria-label',
  'aria-labelledby',
  'aria-level',
  'aria-live',
  'aria-multiline',
  'aria-multiselectable',
  'aria-orientation',
  'aria-owns',
  'aria-placeholder',
  'aria-posinset',
  'aria-pressed',
  'aria-readonly',
  'aria-relevant',
  'aria-required',
  'aria-roledescription',
  'aria-rowcount',

Review comment (suggestion):
Optional: I would prefer not to support deprecated attributes. I see it as us promoting anti-patterns in apps

Code after:
  });

  return attributeObject;
};

/**
 * List of available ARIA attributes + `role`.
 * Removed deprecated attributes.
 * https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes
 */
const ariaAttributes = [
  'role',
  'aria-activedescendant',
  'aria-atomic',
  'aria-autocomplete',
  'aria-braillelabel',
  'aria-brailleroledescription',
  'aria-busy',
  'aria-checked',
  'aria-colcount',
  'aria-colindex',
  'aria-colindextext',
  'aria-colspan',
  'aria-controls',
  'aria-current',
  'aria-describedby',
  'aria-description',
  'aria-details',
  'aria-disabled',
  'aria-errormessage',
  'aria-expanded',
  'aria-flowto',
  'aria-haspopup',
  'aria-hidden',
  'aria-invalid',
  'aria-keyshortcuts',
  'aria-label',
  'aria-labelledby',
  'aria-level',
  'aria-live',
  'aria-multiline',
  'aria-multiselectable',
  'aria-orientation',
  'aria-owns',
  'aria-placeholder',
  'aria-posinset',
  'aria-pressed',
  'aria-readonly',
  'aria-relevant',
  'aria-required',
  'aria-roledescription',
Repository: ionic-team/ionic-framework
File: core/src/components/modal/modal.tsx (TypeScript)

Code before:

    const dismissed = await this.currentTransition;

    if (dismissed) {
      const { delegate } = this.getDelegate();
      await detachComponent(delegate, this.usersElement);

      writeTask(() => this.el.classList.remove('show-modal'));

      if (this.animation) {
        this.animation.destroy();
      }
      if (this.gesture) {
        this.gesture.destroy();
      }

      enteringAnimation.forEach((ani) => ani.destroy());
    }

    if (this.currentBreakpoint) {
      /**
       * If the modal is inline and presented again, the visual breakpoint
       * will not match the current breakpoint. We reset the breakpoint back
       * to the initial breakpoint on dismiss to resolve this issue.
       */
      this.currentBreakpoint = this.initialBreakpoint;
    }

    this.currentTransition = undefined;
    this.animation = undefined;
    return dismissed;
  }

  /**
   * Returns a promise that resolves when the modal did dismiss.
   */
  @Method()
  onDidDismiss<T = any>(): Promise<OverlayEventDetail<T>> {
    return eventMethod(this.el, 'ionModalDidDismiss');
  }

  /**
   * Returns a promise that resolves when the modal will dismiss.
   */
  @Method()
  onWillDismiss<T = any>(): Promise<OverlayEventDetail<T>> {
    return eventMethod(this.el, 'ionModalWillDismiss');
  }

  /**
   * Move a sheet style modal to a specific breakpoint. The breakpoint value must

Review comment (suggestion):
Should this be set on present instead? It might be confusing to have a modal be dismissed and still have a current breakpoint defined.

Code after:

    this.currentTransition = dismiss(this, data, role, 'modalLeave', iosLeaveAnimation, mdLeaveAnimation, {
      presentingEl: this.presentingElement,
      currentBreakpoint: this.currentBreakpoint || this.initialBreakpoint,
      backdropBreakpoint: this.backdropBreakpoint,
    });

    const dismissed = await this.currentTransition;

    if (dismissed) {
      const { delegate } = this.getDelegate();
      await detachComponent(delegate, this.usersElement);

      writeTask(() => this.el.classList.remove('show-modal'));

      if (this.animation) {
        this.animation.destroy();
      }
      if (this.gesture) {
        this.gesture.destroy();
      }

      enteringAnimation.forEach((ani) => ani.destroy());
    }
    this.currentBreakpoint = undefined;
    this.currentTransition = undefined;
    this.animation = undefined;
    return dismissed;
  }

  /**
   * Returns a promise that resolves when the modal did dismiss.
   */
  @Method()
  onDidDismiss<T = any>(): Promise<OverlayEventDetail<T>> {
    return eventMethod(this.el, 'ionModalDidDismiss');
  }

  /**
   * Returns a promise that resolves when the modal will dismiss.
   */
  @Method()
  onWillDismiss<T = any>(): Promise<OverlayEventDetail<T>> {
    return eventMethod(this.el, 'ionModalWillDismiss');
  }

  /**
   * Move a sheet style modal to a specific breakpoint. The breakpoint value must
   * be a value defined in your `breakpoints` array.
   */
  @Method()
Repository: ionic-team/ionic-framework
File: core/src/components/breadcrumbs/test/standalone/breadcrumbs.e2e.ts (TypeScript)

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

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

    // avoid contents getting cut off
    await page.setViewportSize(Viewports.tablet.portrait);

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


Review comment (suggestion):
Are you able to do `page.screenshot({ fullscreen: true })` instead? Since there is no `ion-content` in the standalone file, that should work fine.

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

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

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

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

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

    await page.setIonViewport();

    expect(await page.screenshot({ fullPage: true })).toMatchSnapshot(`button-diff-${page.getSnapshotSettings()}.png`);
  });
});

test.describe('button: ripple effect', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto(`/src/components/button/test/basic?ionic:_testing=false&ionic:mode=md`);

    const button = page.locator('#default');

    await button.scrollIntoViewIfNeeded();

    const boundingBox = await button.boundingBox();

    if (boundingBox) {
      await page.mouse.move(boundingBox.x + boundingBox.width / 2, boundingBox.y + boundingBox.height / 2);
      await page.mouse.down();
    }

    await page.waitForSelector('#default.ion-activated');

    expect(await button.screenshot({ animations: 'disabled' })).toMatchSnapshot(
      `button-ripple-effect-${page.getSnapshotSettings()}.png`
    );
  });
});


Review comment (refactor):
Could we call `test.skip` if `mode !== md` to avoid duplicate test runs, instead of using `ionic:mode=md`? (Right now, the `ios` tests still run, but they're forced to `md`, causing two runs of `md` tests.) Example: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/modal/test/canDismiss/modal.e2e.ts#L138

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

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

    await page.setIonViewport();

    expect(await page.screenshot({ fullPage: true })).toMatchSnapshot(`button-diff-${page.getSnapshotSettings()}.png`);
  });
});

test.describe('button: ripple effect', () => {
  test('should not have visual regressions', async ({ page }, testInfo) => {
    test.skip(testInfo.project.metadata.mode !== 'md', 'Ripple effect is only available in MD mode.');

    await page.goto(`/src/components/button/test/basic?ionic:_testing=false`);

    const button = page.locator('#default');

    await button.scrollIntoViewIfNeeded();

    const boundingBox = await button.boundingBox();

    if (boundingBox) {
      await page.mouse.move(boundingBox.x + boundingBox.width / 2, boundingBox.y + boundingBox.height / 2);
      await page.mouse.down();
    }

    await page.waitForSelector('#default.ion-activated');

    expect(await button.screenshot({ animations: 'disabled' })).toMatchSnapshot(
      `button-ripple-effect-${page.getSnapshotSettings()}.png`
    );
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/button/test/basic/button.e2e.ts (TypeScript)

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

    await page.setIonViewport();

    expect(await page.screenshot({ fullPage: true })).toMatchSnapshot(`button-diff-${page.getSnapshotSettings()}.png`);
  });
});

test.describe('button: ripple effect', () => {
  test('should not have visual regressions', async ({ page }) => {
    await page.goto(`/src/components/button/test/basic?ionic:_testing=false&ionic:mode=md`);

    const button = page.locator('#default');

    await button.scrollIntoViewIfNeeded();

    const boundingBox = await button.boundingBox();

    if (boundingBox) {
      await page.mouse.move(boundingBox.x + boundingBox.width / 2, boundingBox.y + boundingBox.height / 2);
      await page.mouse.down();
    }

    await page.waitForSelector('#default.ion-activated');

    expect(await button.screenshot({ animations: 'disabled' })).toMatchSnapshot(
      `button-ripple-effect-${page.getSnapshotSettings()}.png`
    );
  });
});


Review comment (suggestion):
Shouldn't playwright do this for you automatically?

I know it scrolls the element into view, but I also thought it clicked in the center of the element by default? https://playwright.dev/docs/input#mouse-click

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

    await page.setIonViewport();

    expect(await page.screenshot({ fullPage: true })).toMatchSnapshot(`button-diff-${page.getSnapshotSettings()}.png`);
  });
});

test.describe('button: ripple effect', () => {
  test('should not have visual regressions', async ({ page }, testInfo) => {
    test.skip(testInfo.project.metadata.mode !== 'md', 'Ripple effect is only available in MD mode.');

    await page.goto(`/src/components/button/test/basic?ionic:_testing=false`);

    const button = page.locator('#default');

    await button.scrollIntoViewIfNeeded();

    const boundingBox = await button.boundingBox();

    if (boundingBox) {
      await page.mouse.move(boundingBox.x + boundingBox.width / 2, boundingBox.y + boundingBox.height / 2);
      await page.mouse.down();
    }

    await page.waitForSelector('#default.ion-activated');

    expect(await button.screenshot({ animations: 'disabled' })).toMatchSnapshot(
      `button-ripple-effect-${page.getSnapshotSettings()}.png`
    );
  });
});
Repository: ionic-team/ionic-framework
File: core/src/utils/test/playwright/playwright-page.ts (TypeScript)

Code before:

type CustomFixtures = {
  page: E2EPage;
};

/**
 * Extends the base `page` test figure within Playwright.
 * @param page The page to extend.
 * @param testInfo The test info.
 * @returns The modified playwright page with extended functionality.
 */
export async function extendPageFixture(page: E2EPage, testInfo?: TestInfo) {
  const originalGoto = page.goto.bind(page);
  const originalLocator = page.locator.bind(page);

  // Overridden Playwright methods

  if (testInfo) {
    page.goto = (url: string, options) => goToPage(page, url, options, testInfo, originalGoto);
  }
  page.setContent = (html: string) => setContent(page, html);
  page.locator = (selector: string, options?: LocatorOptions) => locator(page, originalLocator, selector, options);

  // Custom Ionic methods

  if (testInfo) {
    page.getSnapshotSettings = () => getSnapshotSettings(page, testInfo);
  }
  page.setIonViewport = () => setIonViewport(page);
  page.waitForChanges = (timeoutMs?: number) => waitForChanges(page, timeoutMs);
  page.spyOnEvent = (eventName: string) => spyOnEvent(page, eventName);

  // Custom event behavior
  await initPageEvents(page);

  return page;
}

export const test = base.extend<CustomFixtures>({
  page: async ({ page }: CustomTestArgs, use: (r: E2EPage) => Promise<void>, testInfo: TestInfo) => {
    page = await extendPageFixture(page, testInfo);
    await use(page);
  },
});


Review comment (suggestion):
This is confusing. `testInfo` should always be available when running E2E tests in Ionic Core. Additionally, the `E2EPage` interface says that these methods are always available, but that would no longer be the case with this change.

Code after:

type CustomFixtures = {
  page: E2EPage;
};

/**
 * Extends the base `page` test figure within Playwright.
 * @param page The page to extend.
 * @param testInfo The test info.
 * @returns The modified playwright page with extended functionality.
 */
export async function extendPageFixture(page: E2EPage, testInfo: TestInfo) {
  const originalGoto = page.goto.bind(page);
  const originalLocator = page.locator.bind(page);

  // Overridden Playwright methods
  page.goto = (url: string, options) => goToPage(page, url, options, testInfo, originalGoto);
  page.setContent = (html: string) => setContent(page, html);
  page.locator = (selector: string, options?: LocatorOptions) => locator(page, originalLocator, selector, options);

  // Custom Ionic methods
  page.getSnapshotSettings = () => getSnapshotSettings(page, testInfo);
  page.setIonViewport = () => setIonViewport(page);
  page.waitForChanges = (timeoutMs?: number) => waitForChanges(page, timeoutMs);
  page.spyOnEvent = (eventName: string) => spyOnEvent(page, eventName);

  // Custom event behavior
  await initPageEvents(page);

  return page;
}

export const test = base.extend<CustomFixtures>({
  page: async ({ page }: CustomTestArgs, use: (r: E2EPage) => Promise<void>, testInfo: TestInfo) => {
    page = await extendPageFixture(page, testInfo);
    await use(page);
  },
});
Repository: ionic-team/ionic-framework
File: core/src/components/datetime/utils/parse.ts (TypeScript)

Code before:
    }
    if (parse[9] === '-') {
      // + or -
      tzOffset *= -1;
    }
  }

  return {
    year: parse[1],
    month: parse[2],
    day: parse[3],
    hour: parse[4],
    minute: parse[5],
    second: parse[6],
    millisecond: parse[7],
    tzOffset,
  };
};

export const clampDate = (date: string, min?: string, max?: string): string => {
  const parsedDate = new Date(date);
  if (min && parsedDate < new Date(min)) {
    return min;
  } else if (max && parsedDate > new Date(max)) {
    return max;
  }
  return date;
};


Review comment (suggestion):
We should be using `DateTimeParts` to do the comparison, not JS Date as JS Date takes into account different timezones. I believe there are functions in `comparison.ts` you can use.

Code after:
      // minutes
      tzOffset += parseInt(parse[11], 10);
    }
    if (parse[9] === '-') {
      // + or -
      tzOffset *= -1;
    }
  }

  return {
    year: parse[1],
    month: parse[2],
    day: parse[3],
    hour: parse[4],
    minute: parse[5],
    second: parse[6],
    millisecond: parse[7],
    tzOffset,
  };
};

export const clampDate = (
  dateParts: DatetimeParts,
  minParts?: DatetimeParts,
  maxParts?: DatetimeParts
): DatetimeParts => {
  if (minParts && isBefore(dateParts, minParts)) {
    return minParts;
  } else if (maxParts && isAfter(dateParts, maxParts)) {
    return maxParts;
  }
  return dateParts;
};
Repository: ionic-team/ionic-framework
File: core/src/components/datetime/utils/parse.ts (TypeScript)

Code before:
  if (parse[9] && parse[10]) {
    // hours
    tzOffset = parseInt(parse[10], 10) * 60;
    if (parse[11]) {
      // minutes
      tzOffset += parseInt(parse[11], 10);
    }
    if (parse[9] === '-') {
      // + or -
      tzOffset *= -1;
    }
  }

  return {
    year: parse[1],
    month: parse[2],
    day: parse[3],
    hour: parse[4],
    minute: parse[5],
    second: parse[6],
    millisecond: parse[7],
    tzOffset,
  };
};

export const clampDate = (date: string, min?: string, max?: string): string => {
  const dateParts = parseDate(date);
  if (min) {
    const minParts = parseDate(min);
    if (isBefore(dateParts, minParts)) {
      return min;
    }
  }
  if (max) {
    const maxParts = parseDate(max);
    if (isAfter(dateParts, maxParts)) {
      return max;
    }
  }
  return date;
};


Review comment (suggestion):
By the time `processValue` is called, we have already parsed the `min` and `max` values, so we should be able to pass in the `DatetimeParts` instead of the strings: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/datetime/datetime.tsx#L1111-L1113

Code after:
  if (parse[9] && parse[10]) {
    // hours
    tzOffset = parseInt(parse[10], 10) * 60;
    if (parse[11]) {
      // minutes
      tzOffset += parseInt(parse[11], 10);
    }
    if (parse[9] === '-') {
      // + or -
      tzOffset *= -1;
    }
  }

  return {
    year: parse[1],
    month: parse[2],
    day: parse[3],
    hour: parse[4],
    minute: parse[5],
    second: parse[6],
    millisecond: parse[7],
    tzOffset,
  };
};

export const clampDate = (
  dateParts: DatetimeParts,
  minParts?: DatetimeParts,
  maxParts?: DatetimeParts
): DatetimeParts => {
  if (minParts && isBefore(dateParts, minParts)) {
    return minParts;
  } else if (maxParts && isAfter(dateParts, maxParts)) {
    return maxParts;
  }
  return dateParts;
};
Repository: ionic-team/ionic-framework
File: core/src/components/datetime/utils/parse.ts (TypeScript)

Code before:
    // hours
    tzOffset = parseInt(parse[10], 10) * 60;
    if (parse[11]) {
      // minutes
      tzOffset += parseInt(parse[11], 10);
    }
    if (parse[9] === '-') {
      // + or -
      tzOffset *= -1;
    }
  }

  return {
    year: parse[1],
    month: parse[2],
    day: parse[3],
    hour: parse[4],
    minute: parse[5],
    second: parse[6],
    millisecond: parse[7],
    tzOffset,
  };
};

export const clampDate = (date: string, min?: string, max?: string): string => {
  const dateParts = parseDate(date);
  if (min) {
    const minParts = parseDate(min);
    if (isBefore(dateParts, minParts)) {
      return min;
    }
  }
  if (max) {
    const maxParts = parseDate(max);
    if (isAfter(dateParts, maxParts)) {
      return max;
    }
  }
  return date;
};


Review comment (suggestion):
You could also optionally parse `date` in `datetime.tsx` so that this function takes in `DatetimeParts` across the board, but I don't have any strong opinions on this. The one benefit I see is consistency of arguments given that they are all dates.

Code after:
    // hours
    tzOffset = parseInt(parse[10], 10) * 60;
    if (parse[11]) {
      // minutes
      tzOffset += parseInt(parse[11], 10);
    }
    if (parse[9] === '-') {
      // + or -
      tzOffset *= -1;
    }
  }

  return {
    year: parse[1],
    month: parse[2],
    day: parse[3],
    hour: parse[4],
    minute: parse[5],
    second: parse[6],
    millisecond: parse[7],
    tzOffset,
  };
};

export const clampDate = (
  dateParts: DatetimeParts,
  minParts?: DatetimeParts,
  maxParts?: DatetimeParts
): DatetimeParts => {
  if (minParts && isBefore(dateParts, minParts)) {
    return minParts;
  } else if (maxParts && isAfter(dateParts, maxParts)) {
    return maxParts;
  }
  return dateParts;
};