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

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

test.describe('datetime-button: disabled buttons', () => {
  // eslint-disable-next-line no-empty-pattern
  test.beforeEach(async ({}, testInfo) => {
    test.skip(testInfo.project.metadata.rtl === 'rtl', 'No layout tests');
    test.skip(testInfo.project.metadata.mode === 'ios', 'No mode-specific logic');
  });
  test('buttons should not be enabled when component is disabled', async ({ page }) => {
    await page.setContent(`
      <ion-datetime-button datetime="datetime" disabled="true"></ion-datetime-button>
      <ion-datetime id="datetime" presentation="date-time"></ion-datetime>
    `);
    await page.waitForSelector('.datetime-ready');

    await expect(page.locator('#date-button')).toBeDisabled();
    await expect(page.locator('#time-button')).toBeDisabled();
  });
  test('buttons should visually be disabled', async ({ page }) => {
    await page.setContent(`
      <ion-datetime-button datetime="datetime" disabled="true"></ion-datetime-button>
      <ion-datetime id="datetime" presentation="date-time"></ion-datetime>
    `);
    await page.waitForSelector('.datetime-ready');

    const datetimeButton = page.locator('ion-datetime-button');
    expect(await datetimeButton.screenshot()).toMatchSnapshot(
      `datetime-button-disabled-${page.getSnapshotSettings()}.png`
    );
  });
});


Review comment (suggestion):
Since we have a screenshot test now, it may be wise to move these skips to only cover the `toBeDisabled` test.

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

test.describe('datetime-button: disabled buttons', () => {
  test('buttons should not be enabled when component is disabled', async ({ page }, testInfo) => {
    test.skip(testInfo.project.metadata.rtl === 'rtl', 'No layout tests');
    test.skip(testInfo.project.metadata.mode === 'ios', 'No mode-specific logic');

    await page.setContent(`
      <ion-datetime-button datetime="datetime" disabled="true"></ion-datetime-button>
      <ion-datetime id="datetime" presentation="date-time"></ion-datetime>
    `);
    await page.waitForSelector('.datetime-ready');

    await expect(page.locator('#date-button')).toBeDisabled();
    await expect(page.locator('#time-button')).toBeDisabled();
  });
  test('buttons should visually be disabled', async ({ page }) => {
    await page.setContent(`
      <ion-datetime-button datetime="datetime" disabled="true"></ion-datetime-button>
      <ion-datetime id="datetime" presentation="date-time" value="2022-01-01T16:30:00"></ion-datetime>
    `);
    await page.waitForSelector('.datetime-ready');

    const datetimeButton = page.locator('ion-datetime-button');
    expect(await datetimeButton.screenshot()).toMatchSnapshot(
      `datetime-button-disabled-${page.getSnapshotSettings()}.png`
    );
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/datetime/utils/format.ts (TypeScript)

Code before:
import { convertDataToISO } from './manipulation';

const getFormattedDayPeriod = (dayPeriod?: string) => {
  if (dayPeriod === undefined) {
    return '';
  }

  return dayPeriod.toUpperCase();
};

export const getLocalizedTime = (locale: string, refParts: DatetimeParts, use24Hour: boolean): string => {
  if (refParts.hour === undefined || refParts.minute === undefined) {
    return 'Invalid Time';
  }

  return new Intl.DateTimeFormat(locale, {
    hour: 'numeric',
    minute: 'numeric',
    timeZone: 'UTC',
    hour12: !use24Hour,
  }).format(
    new Date(
      convertDataToISO({
        ...refParts,
        tzOffset: undefined,
      })
    )
  );
};

/**
 * Adds padding to a time value so
 * that it is always 2 digits.
 */
export const addTimePadding = (value: number): string => {
  const valueToString = value.toString();
  if (valueToString.length > 1) {
    return valueToString;
  }

  return `0${valueToString}`;
};

/**
 * Formats the hour value so that it
 * is always 2 digits. Only applies
 * if using 12 hour format.
 */
export const getFormattedHour = (hour: number, use24Hour: boolean): string => {
  if (!use24Hour) {
    return hour.toString();

Review comment (refactor):
When fixing FW-1831, would this issue also get resolved? The solution for that issue may be to remove any timezone processing from `convertDataToISO`. If this issue would get resolved, it might be worth adding a TODO to make sure we remove this additional line since it would be redundant.

Code after:
import { convertDataToISO } from './manipulation';

const getFormattedDayPeriod = (dayPeriod?: string) => {
  if (dayPeriod === undefined) {
    return '';
  }

  return dayPeriod.toUpperCase();
};

export const getLocalizedTime = (locale: string, refParts: DatetimeParts, use24Hour: boolean): string => {
  if (refParts.hour === undefined || refParts.minute === undefined) {
    return 'Invalid Time';
  }

  return new Intl.DateTimeFormat(locale, {
    hour: 'numeric',
    minute: 'numeric',
    timeZone: 'UTC',
    hour12: !use24Hour,
  }).format(
    new Date(
      convertDataToISO({
        ...refParts,
        // TODO: FW-1831 will remove the need to manually set the tzOffset to undefined
        tzOffset: undefined,
      })
    )
  );
};

/**
 * Adds padding to a time value so
 * that it is always 2 digits.
 */
export const addTimePadding = (value: number): string => {
  const valueToString = value.toString();
  if (valueToString.length > 1) {
    return valueToString;
  }

  return `0${valueToString}`;
};

/**
 * Formats the hour value so that it
 * is always 2 digits. Only applies
 * if using 12 hour format.
 */
export const getFormattedHour = (hour: number, use24Hour: boolean): string => {
  if (!use24Hour) {
Repository: ionic-team/ionic-framework
File: core/src/components/searchbar/test/basic/searchbar.e2e.ts (TypeScript)

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

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

  test('should not have visual regressions', async ({ page }) => {
    await page.setIonViewport();
    expect(await page.screenshot()).toMatchSnapshot(`searchbar-diff-${page.getSnapshotSettings()}.png`);
  });

  test('should show cancel button on focus if show-cancel-button=focus', async ({ page }) => {
    const searchbar = page.locator('#basic');

    await searchbar.evaluate((el: HTMLIonSearchbarElement) => el.setFocus());
    await page.waitForChanges();

    expect(searchbar).toHaveClass(/searchbar-has-focus/);
    expect(await searchbar.screenshot()).toMatchSnapshot(
      `searchbar-show-cancel-focus-${page.getSnapshotSettings()}.png`
    );
  });

  test('should not show cancel button on focus if show-cancel-button=never', async ({ page }) => {
    const searchbar = page.locator('#noCancel');

    await searchbar.evaluate((el: HTMLIonSearchbarElement) => el.setFocus());
    await page.waitForChanges();

    expect(searchbar).toHaveClass(/searchbar-has-focus/);
    expect(await searchbar.screenshot()).toMatchSnapshot(
      `searchbar-show-cancel-never-${page.getSnapshotSettings()}.png`
    );
  });
});


Review comment (bug):
~ It may help to put `await` on these assertions, in an odd event that the `waitForChanges` threshold doesn't fully re-render with the applied classes. It will then await up to another 5000ms before failing. 

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

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

  test('should not have visual regressions', async ({ page }) => {
    await page.setIonViewport();
    expect(await page.screenshot()).toMatchSnapshot(`searchbar-diff-${page.getSnapshotSettings()}.png`);
  });

  test('should show cancel button on focus if show-cancel-button=focus', async ({ page }) => {
    const searchbar = page.locator('#basic');
    const cancelButton = searchbar.locator('.searchbar-cancel-button');

    await searchbar.evaluate((el: HTMLIonSearchbarElement) => el.setFocus());
    await page.waitForChanges();

    await expect(searchbar).toHaveClass(/searchbar-has-focus/);
    await expect(cancelButton).toBeVisible();
  });

  test('should not show cancel button on focus if show-cancel-button=never', async ({ page }) => {
    const searchbar = page.locator('#noCancel');
    const cancelButton = searchbar.locator('.searchbar-cancel-button');

    await searchbar.evaluate((el: HTMLIonSearchbarElement) => el.setFocus());
    await page.waitForChanges();

    await expect(searchbar).toHaveClass(/searchbar-has-focus/);
    await expect(cancelButton).toHaveCount(0);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/radio-group/test/form/radio-group.e2e.ts (TypeScript)

Code before:
test.describe('radio-group: form', () => {
  test.beforeEach(async ({ page }) => {
    await page.goto('/src/components/radio-group/test/form');
  });

  test('selecting an option should update the value', async ({ page }) => {
    const value = page.locator('#value');
    const griffRadio = page.locator('ion-radio[value="griff"]');

    await expect(value).toHaveText('');

    await griffRadio.click();

    await expect(value).toHaveText('griff');
  });

  test('selecting a disabled option should not update the value', async ({ page }) => {
    const value = page.locator('#value');
    const disabledRadio = page.locator('ion-radio[value="george"]');

    await expect(value).toHaveText('');
    await expect(disabledRadio).toHaveAttribute('disabled', '');

    await disabledRadio.click({ force: true });

    await expect(value).toHaveText('');
  });
});


Review comment (suggestion):
I don't think this would reliably catch a regression because we don't wait for Stencil to update the state of the component before checking the value. We may need to do `await page.waitForChanges()` after we click.

Code after:
test.describe('radio-group: form', () => {
  test.beforeEach(async ({ page }) => {
    await page.goto('/src/components/radio-group/test/form');
  });

  test('selecting an option should update the value', async ({ page }) => {
    const radioGroup = page.locator('ion-radio-group');
    const ionChange = await page.spyOnEvent('ionChange');
    const griffRadio = page.locator('ion-radio[value="griff"]');

    await expect(radioGroup).toHaveAttribute('value', 'biff');

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

    await expect(ionChange).toHaveReceivedEventDetail({ value: 'griff' });
  });

  test('selecting a disabled option should not update the value', async ({ page }) => {
    const value = page.locator('#value');
    const disabledRadio = page.locator('ion-radio[value="george"]');

    await expect(value).toHaveText('');
    await expect(disabledRadio).toHaveAttribute('disabled', '');

    await disabledRadio.click({ force: true });
    await page.waitForChanges();

    await expect(value).toHaveText('');
  });
});
Repository: ionic-team/ionic-framework
File: core/src/utils/keyboard/keyboard-controller.ts (TypeScript)

Code before:
import { win } from '@utils/window';

/**
 * Creates a controller that tracks and reacts to opening or closing the keyboard.
 *
 * @internal
 * @param keyboardChangeCallback A function to call when the keyboard opens or closes.
 */
export const createKeyboardController = (keyboardChangeCallback?: (keyboardOpen: boolean) => void) => {
  let keyboardWillShowHandler: (() => void) | undefined;
  let keyboardWillHideHandler: (() => void) | undefined;
  let keyboardVisible: boolean;

  const init = () => {
    keyboardWillShowHandler = () => {
      keyboardVisible = true;
      if (keyboardChangeCallback) keyboardChangeCallback(true);
    };

    keyboardWillHideHandler = () => {
      keyboardVisible = false;
      if (keyboardChangeCallback) keyboardChangeCallback(false);
    };

    win?.addEventListener('keyboardWillShow', keyboardWillShowHandler);
    win?.addEventListener('keyboardWillHide', keyboardWillHideHandler);

Review comment (suggestion):
We can't use relative imports in non-test code until https://github.com/ionic-team/stencil/issues/2815 is resolved.

Code after:
import { win } from '../window';

/**
 * Creates a controller that tracks and reacts to opening or closing the keyboard.
 *
 * @internal
 * @param keyboardChangeCallback A function to call when the keyboard opens or closes.
 */
export const createKeyboardController = (
  keyboardChangeCallback?: (keyboardOpen: boolean) => void
): KeyboardController => {
  let keyboardWillShowHandler: (() => void) | undefined;
  let keyboardWillHideHandler: (() => void) | undefined;
  let keyboardVisible: boolean;

  const init = () => {
    keyboardWillShowHandler = () => {
      keyboardVisible = true;
      if (keyboardChangeCallback) keyboardChangeCallback(true);
    };

    keyboardWillHideHandler = () => {
      keyboardVisible = false;
      if (keyboardChangeCallback) keyboardChangeCallback(false);
    };
Repository: ionic-team/ionic-framework
File: core/src/components/footer/footer.tsx (TypeScript)

Code before:
import type { ComponentInterface } from '@stencil/core';
import { Component, Element, Host, Prop, State, h } from '@stencil/core';
import type { KeyboardController } from '@utils/keyboard/keyboard-controller';
import { createKeyboardController } from '@utils/keyboard/keyboard-controller';

import { getIonMode } from '../../global/ionic-global';
import { findIonContent, getScrollElement, printIonContentErrorMsg } from '../../utils/content';

import { handleFooterFade } from './footer.utils';

/**
 * @virtualProp {"ios" | "md"} mode - The mode determines which platform styles to use.
 */
@Component({
  tag: 'ion-footer',
  styleUrls: {
    ios: 'footer.ios.scss',
    md: 'footer.md.scss',
  },
})
export class Footer implements ComponentInterface {
  private scrollEl?: HTMLElement;
  private contentScrollCallback: any;
  private keyboardCtrl: KeyboardController | null = null;

  @State() private keyboardVisible = false;

  @Element() el!: HTMLIonFooterElement;


Review comment (suggestion):
We can't use relative imports in non-test code until https://github.com/ionic-team/stencil/issues/2815 is resolved.

Code after:
import type { ComponentInterface } from '@stencil/core';
import { Component, Element, Host, Prop, State, h } from '@stencil/core';

import { getIonMode } from '../../global/ionic-global';
import { findIonContent, getScrollElement, printIonContentErrorMsg } from '../../utils/content';
import type { KeyboardController } from '../../utils/keyboard/keyboard-controller';
import { createKeyboardController } from '../../utils/keyboard/keyboard-controller';

import { handleFooterFade } from './footer.utils';

/**
 * @virtualProp {"ios" | "md"} mode - The mode determines which platform styles to use.
 */
@Component({
  tag: 'ion-footer',
  styleUrls: {
    ios: 'footer.ios.scss',
    md: 'footer.md.scss',
  },
})
export class Footer implements ComponentInterface {
  private scrollEl?: HTMLElement;
  private contentScrollCallback: any;
  private keyboardCtrl: KeyboardController | null = null;

  @State() private keyboardVisible = false;

  @Element() el!: HTMLIonFooterElement;
Repository: ionic-team/ionic-framework
File: angular/src/directives/control-value-accessors/numeric-value-accessor.ts (TypeScript)

Code before:
import { Directive, HostListener, ElementRef, Injector } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';

import { ValueAccessor } from './value-accessor';

@Directive({
  selector: 'ion-input[type=number]',
  providers: [
    {
      provide: NG_VALUE_ACCESSOR,
      useExisting: NumericValueAccessorDirective,
      multi: true,
    },
  ],
})
export class NumericValueAccessorDirective extends ValueAccessor {
  constructor(injector: Injector, el: ElementRef) {
    super(injector, el);
  }

  @HostListener('ionInput', ['$event.target'])
  handleInputEvent(el: HTMLIonInputElement): void {
    this.handleChangeEvent(el, el.value);
  }

  registerOnChange(fn: (_: number | null) => void): void {
    super.registerOnChange((value: string) => {
      fn(value === '' ? null : parseFloat(value));
    });
  }
}


Review comment (refactor):
This seems confusing; it looks like we're emitting `ionChange` at the same time as `ionInput`. Is there any way to refactor or comment this to be more clear at first glance? (Ditto for the similar change in `text-value-accessor.ts`.)

Code after:
import { Directive, HostListener, ElementRef, Injector } from '@angular/core';
import { NG_VALUE_ACCESSOR } from '@angular/forms';

import { ValueAccessor } from './value-accessor';

@Directive({
  selector: 'ion-input[type=number]',
  providers: [
    {
      provide: NG_VALUE_ACCESSOR,
      useExisting: NumericValueAccessorDirective,
      multi: true,
    },
  ],
})
export class NumericValueAccessorDirective extends ValueAccessor {
  constructor(injector: Injector, el: ElementRef) {
    super(injector, el);
  }

  @HostListener('ionInput', ['$event.target'])
  handleInputEvent(el: HTMLIonInputElement): void {
    this.handleValueChange(el, el.value);
  }

  registerOnChange(fn: (_: number | null) => void): void {
    super.registerOnChange((value: string) => {
      fn(value === '' ? null : parseFloat(value));
    });
  }
}
Repository: ionic-team/ionic-framework
File: core/src/components/input/input.tsx (TypeScript)

Code before:
  private onCompositionEnd = () => {
    this.isComposing = false;
  };

  private clearTextOnEnter = (ev: KeyboardEvent) => {
    if (ev.key === 'Enter') {
      this.clearTextInput(ev);
    }
  };

  private clearTextInput = (ev?: Event) => {
    if (this.clearInput && !this.readonly && !this.disabled && ev) {
      ev.preventDefault();
      ev.stopPropagation();

      // Attempt to focus input again after pressing clear button
      this.setFocus();
    }

    this.value = '';

    /**
     * Clearing the text input is considered a user committed change
     * and should emit a value change event.
     */
    this.emitValueChange();

    /**
     * This is needed for clearOnEdit
     * Otherwise the value will not be cleared
     * if user is inside the input
     */
    if (this.nativeInput) {
      this.nativeInput.value = '';
    }
  };

  private focusChanged() {
    // If clearOnEdit is enabled and the input blurred but has a value, set a flag
    if (!this.hasFocus && this.shouldClearOnEdit() && this.hasValue()) {
      this.didBlurAfterEdit = true;
    }
  }

  private hasValue(): boolean {
    return this.getValue().length > 0;
  }

  render() {
    const mode = getIonMode(this);
    const value = this.getValue();

Review comment (suggestion):
Should this happen on blur instead? Currently, the native `change` event fires when a change is committed.

The clear button on `input type="search"` in Chrome emits on blur: https://codepen.io/liamdebeasi/pen/vYjYEJv

Code after:
      if (this.focusedValue !== this.value) {
        /**
         * Emits the `ionChange` event when the input value
         * is different than the value when the input was focused.
         */
        this.emitValueChange();
      }
      this.focusedValue = undefined;
      this.inputCleared = false;
    }

    this.ionBlur.emit(ev);
  };

  private onFocus = (ev: FocusEvent) => {
    this.hasFocus = true;
    this.focusedValue = this.value;
    this.focusChanged();
    this.emitStyle();

    this.ionFocus.emit(ev);
  };

  private onKeydown = (ev: KeyboardEvent) => {
    if (this.shouldClearOnEdit()) {
      // Did the input value change after it was blurred and edited?
      // Do not clear if user is hitting Enter to submit form
      if (this.didBlurAfterEdit && this.hasValue() && ev.key !== 'Enter') {
        // Clear the input
        this.clearTextInput();
      }

      // Reset the flag
      this.didBlurAfterEdit = false;
    }
  };

  private onCompositionStart = () => {
    this.isComposing = true;
  };

  private onCompositionEnd = () => {
    this.isComposing = false;
  };

  private clearTextInput = (ev?: Event) => {
    if (this.clearInput && !this.readonly && !this.disabled && ev) {
      ev.preventDefault();
      ev.stopPropagation();

      // Attempt to focus input again after pressing clear button
Repository: ionic-team/ionic-framework
File: core/src/components/input/input.tsx (TypeScript)

Code before:
  };

  private onCompositionEnd = () => {
    this.isComposing = false;
  };

  private clearTextOnEnter = (ev: KeyboardEvent) => {
    if (ev.key === 'Enter') {
      this.clearTextInput(ev);
    }
  };

  private clearTextInput = (ev?: Event) => {
    if (this.clearInput && !this.readonly && !this.disabled && ev) {
      ev.preventDefault();
      ev.stopPropagation();

      // Attempt to focus input again after pressing clear button
      this.setFocus();
    }

    this.value = '';

    this.ionInput.emit({
      ...ev,
      data: this.value,
    } as InputEvent);

    /**
     * This is needed for clearOnEdit
     * Otherwise the value will not be cleared
     * if user is inside the input
     */
    if (this.nativeInput) {
      this.nativeInput.value = '';
    }
  };

  private focusChanged() {
    // If clearOnEdit is enabled and the input blurred but has a value, set a flag
    if (!this.hasFocus && this.shouldClearOnEdit() && this.hasValue()) {
      this.didBlurAfterEdit = true;
    }
  }

  private hasValue(): boolean {
    return this.getValue().length > 0;
  }

  render() {
    const mode = getIonMode(this);

Review comment (suggestion):
I'm comparing this with the native behavior in Chrome, and noticed that it does not pass `data` when pressing the "X" button. It only seems to pass `data` when typing. Should we align the behavior here? One thing to note is that the native event is of type `Event`, not `InputEvent`.

CodePen: https://codepen.io/liamdebeasi/pen/oNdXJrp

Code after:

    if (this.inputCleared) {
      if (this.focusedValue !== this.value) {
        /**
         * Emits the `ionChange` event when the input value
         * is different than the value when the input was focused.
         */
        this.emitValueChange();
      }
      this.focusedValue = undefined;
      this.inputCleared = false;
    }

    this.ionBlur.emit(ev);
  };

  private onFocus = (ev: FocusEvent) => {
    this.hasFocus = true;
    this.focusedValue = this.value;
    this.focusChanged();
    this.emitStyle();

    this.ionFocus.emit(ev);
  };

  private onKeydown = (ev: KeyboardEvent) => {
    if (this.shouldClearOnEdit()) {
      // Did the input value change after it was blurred and edited?
      // Do not clear if user is hitting Enter to submit form
      if (this.didBlurAfterEdit && this.hasValue() && ev.key !== 'Enter') {
        // Clear the input
        this.clearTextInput();
      }

      // Reset the flag
      this.didBlurAfterEdit = false;
    }
  };

  private onCompositionStart = () => {
    this.isComposing = true;
  };

  private onCompositionEnd = () => {
    this.isComposing = false;
  };

  private clearTextInput = (ev?: Event) => {
    if (this.clearInput && !this.readonly && !this.disabled && ev) {
      ev.preventDefault();
      ev.stopPropagation();