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

Code before:
    }
  };

  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
      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);
    const value = this.getValue();

Review comment (suggestion):
The underlying event is not actually an `InputEvent`. We should probably update the event type information instead of typecasting this. It looks like native can emit `InputEvent | Event`.

Code after:

    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
      this.setFocus();
    }

    this.value = '';
    this.inputCleared = true;

    this.ionInput.emit(ev);

    /**
     * This is needed for clearOnEdit
Repository: ionic-team/ionic-framework
File: packages/vue/test/base/tests/unit/router-outlet.spec.ts (TypeScript)

Code before:
  IonApp,
  IonRouterOutlet,
  IonPage,
  useIonRouter,
  createAnimation
} from '@ionic/vue';
import { onBeforeRouteLeave } from 'vue-router';
import { mockAnimation, waitForRouter } from './utils';

enableAutoUnmount(afterEach);

const App = {
  components: { IonApp, IonRouterOutlet },
  template: '<ion-app><ion-router-outlet /></ion-app>',
}

const BasePage = {
  template: '<ion-page :data-pageid="name"></ion-page>',
  components: { IonPage },
}

/**
 * While these tests use useIonRouter,
 * they are different from the tests in hook.spec.ts
 * in that they are testing that the correct parameters
 * are passed to IonRouterOutlet as opposed hook.spec.ts
 * which makes sure that the animation function is called when
 * specifically using useIonRouter.
 */
describe('Routing', () => {
  it('should have an animation duration of 0 if replacing without an explicit animation', async () => {
    const Page1 = {
      ...BasePage,
      setup() {
        const ionRouter = useIonRouter();
        const redirect = () => {
          ionRouter.replace('/page2')
        }

        return { redirect }
      }
    };

    const Page2 = {
      ...BasePage
    };

    const router = createRouter({
      history: createWebHistory(process.env.BASE_URL),
      routes: [
        { path: '/', component: Page1 },

Review comment (suggestion):
```suggestion
 * are passed to IonRouterOutlet as opposed to hook.spec.ts
```

Code after:
  IonApp,
  IonRouterOutlet,
  IonPage,
  useIonRouter,
  createAnimation
} from '@ionic/vue';
import { onBeforeRouteLeave } from 'vue-router';
import { mockAnimation, waitForRouter } from './utils';

enableAutoUnmount(afterEach);

const App = {
  components: { IonApp, IonRouterOutlet },
  template: '<ion-app><ion-router-outlet /></ion-app>',
}

const BasePage = {
  template: '<ion-page :data-pageid="name"></ion-page>',
  components: { IonPage },
}

/**
 * While these tests use useIonRouter,
 * they are different from the tests in hook.spec.ts
 * in that they are testing that the correct parameters
 * are passed to IonRouterOutlet as opposed to hook.spec.ts
 * which makes sure that the animation function is called when
 * specifically using useIonRouter.
 */
describe('Routing', () => {
  it('should have an animation duration of 0 if replacing without an explicit animation', async () => {
    const Page1 = {
      ...BasePage,
      setup() {
        const ionRouter = useIonRouter();
        const redirect = () => {
          ionRouter.replace('/page2')
        }

        return { redirect }
      }
    };

    const Page2 = {
      ...BasePage
    };

    const router = createRouter({
      history: createWebHistory(process.env.BASE_URL),
      routes: [
        { path: '/', component: Page1 },
Repository: ionic-team/ionic-framework
File: core/src/components/item/test/highlight/item.e2e.ts (TypeScript)

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

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

    await page.setIonViewport({ resizeViewportWidth: true });

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


Review comment (suggestion):
Question for this configuration used in both tests - were you observing horizontal clipping of the screenshot viewport? 

Based on the screenshot captures, I think this can likely be removed if not.

```ts
await page.setIonViewport();
```

The `resizeViewportWidth` option was introduced for 1 item test we have, where the rendered contents are larger than the device width (some legacy test that renders on like a landscape tablet viewport width). 

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

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

    await page.setIonViewport();

    expect(await page.screenshot()).toMatchSnapshot(`item-highlight-diff-${page.getSnapshotSettings()}.png`);
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/segment/segment.tsx (TypeScript)

Code before:
  onStart(detail: GestureDetail) {
    this.activate(detail);
  }

  onMove(detail: GestureDetail) {
    this.setNextIndex(detail);
  }

  onEnd(detail: GestureDetail) {
    this.setActivated(false);

    const checkedValidButton = this.setNextIndex(detail, true);

    detail.event.stopImmediatePropagation();

    if (checkedValidButton) {
      this.addRipple(detail);
    }

    const value = this.valueAfterGesture;
    if (value !== undefined) {
      this.valueAfterGesture = undefined;
    }
  }

  /**
   * Emits an `ionChange` event.
   *
   * This API should be called for user committed changes.
   * This API should not be used for external value changes.
   */
  private emitValueChange() {
    const { value } = this;
    this.ionChange.emit({ value });
  }

  private getButtons() {
    return Array.from(this.el.querySelectorAll('ion-segment-button'));
  }

  /**
   * The gesture blocks the segment button ripple. This
   * function adds the ripple based on the checked segment
   * and where the cursor ended.
   */
  private addRipple(detail: GestureDetail) {
    const useRippleEffect = config.getBoolean('animated', true) && config.getBoolean('rippleEffect', true);
    if (!useRippleEffect) {
      return;
    }


Review comment (refactor):
If you compare the changes side-by-side, you might think `ionChange` is no longer emitted here. While I did remove this event emission, the earlier function calls execute `setNextIndex` → `checkButton` → `ionChange.emit`.

It was removed at this point as it was redundant and causing an extra event emission (once `ionChange.emit` was introduced to `checkButton`). 

Code after:

  onStart(detail: GestureDetail) {
    this.valueBeforeGesture = this.value;
    this.activate(detail);
  }

  onMove(detail: GestureDetail) {
    this.setNextIndex(detail);
  }

  onEnd(detail: GestureDetail) {
    this.setActivated(false);

    const checkedValidButton = this.setNextIndex(detail, true);

    detail.event.stopImmediatePropagation();

    if (checkedValidButton) {
      this.addRipple(detail);
    }

    const value = this.value;
    if (value !== undefined) {
      if (this.valueBeforeGesture !== value) {
        this.emitValueChange();
      }
    }
    this.valueBeforeGesture = undefined;
  }

  /**
   * Emits an `ionChange` event.
   *
   * This API should be called for user committed changes.
   * This API should not be used for external value changes.
   */
  private emitValueChange() {
    const { value } = this;
    this.ionChange.emit({ value });
  }

  private getButtons() {
    return Array.from(this.el.querySelectorAll('ion-segment-button'));
  }

  /**
   * The gesture blocks the segment button ripple. This
   * function adds the ripple based on the checked segment
   * and where the cursor ended.
   */
  private addRipple(detail: GestureDetail) {
Repository: ionic-team/ionic-framework
File: core/src/components/segment/segment.tsx (TypeScript)

Code before:
   * If `true`, the segment buttons will overflow and the user can swipe to see them.
   * In addition, this will disable the gesture to drag the indicator between the buttons
   * in order to swipe to see hidden buttons.
   */
  @Prop() scrollable = false;

  /**
   * If `true`, users will be able to swipe between segment buttons to activate them.
   */
  @Prop() swipeGesture = true;

  @Watch('swipeGesture')
  swipeGestureChanged() {
    this.gestureChanged();
  }

  /**
   * the value of the segment.
   */
  @Prop({ mutable: true }) value?: string;

  @Watch('value')
  protected valueChanged(value: string | undefined, oldValue: string | undefined | null) {
    this.ionSelect.emit({ value });
    if (oldValue !== '') {
      if (this.activated) {
        this.valueAfterGesture = value;
      }
    }
  }

  /**
   * If `true`, navigating to an `ion-segment-button` with the keyboard will focus and select the element.
   * If `false`, keyboard navigation will only focus the `ion-segment-button` element.
   */
  @Prop() selectOnFocus = false;

  /**
   * Emitted when the value property has changed and any
   * dragging pointer has been released from `ion-segment`.
   */
  @Event() ionChange!: EventEmitter<SegmentChangeEventDetail>;

  /**
   * Emitted when user has dragged over a new button
   * @internal
   */
  @Event() ionSelect!: EventEmitter<SegmentChangeEventDetail>;

  /**
   * Emitted when the styles change.

Review comment (suggestion):
```suggestion
    if (oldValue !== '' && this.activated) {
```

Code after:
   * If `true`, the segment buttons will overflow and the user can swipe to see them.
   * In addition, this will disable the gesture to drag the indicator between the buttons
   * in order to swipe to see hidden buttons.
   */
  @Prop() scrollable = false;

  /**
   * If `true`, users will be able to swipe between segment buttons to activate them.
   */
  @Prop() swipeGesture = true;

  @Watch('swipeGesture')
  swipeGestureChanged() {
    this.gestureChanged();
  }

  /**
   * the value of the segment.
   */
  @Prop({ mutable: true }) value?: string;

  @Watch('value')
  protected valueChanged(value: string | undefined) {
    /**
     * `ionSelect` is emitted every time the value changes (internal or external changes).
     * Used by `ion-segment-button` to determine if the button should be checked.
     */
    this.ionSelect.emit({ value });
  }

  /**
   * If `true`, navigating to an `ion-segment-button` with the keyboard will focus and select the element.
   * If `false`, keyboard navigation will only focus the `ion-segment-button` element.
   */
  @Prop() selectOnFocus = false;

  /**
   * Emitted when the value property has changed and any
   * dragging pointer has been released from `ion-segment`.
   */
  @Event() ionChange!: EventEmitter<SegmentChangeEventDetail>;

  /**
   * Emitted when the value of the segment changes from user committed actions
   * or from externally assigning a value.
   *
   * @internal
   */
  @Event() ionSelect!: EventEmitter<SegmentChangeEventDetail>;

  /**
Repository: ionic-team/ionic-framework
File: core/src/components/fab-button/test/a11y/fab-button.e2e.ts (TypeScript)

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

test.describe('fab-button: aria attributes', () => {
  test('should inherit aria attributes to inner button', async ({ page, skip }) => {
    skip.rtl();
    skip.mode('ios');

    await page.setContent(`
      <ion-fab-button aria-label="Hello World">My Button</ion-fab-button>
    `);

    const nativeButton = page.locator('ion-fab-button button');
    expect(nativeButton).toHaveAttribute('aria-label', 'Hello World');
  });
});


Review comment (suggestion):
Whoops just saw this
```suggestion
    await expect(nativeButton).toHaveAttribute('aria-label', 'Hello World');
```

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

test.describe('fab-button: aria attributes', () => {
  test('should inherit aria attributes to inner button', async ({ page, skip }) => {
    skip.rtl();
    skip.mode('ios');

    await page.setContent(`
      <ion-fab-button aria-label="Hello World">My Button</ion-fab-button>
    `);

    const nativeButton = page.locator('ion-fab-button button');
    await expect(nativeButton).toHaveAttribute('aria-label', 'Hello World');
  });
});
Repository: ionic-team/ionic-framework
File: core/src/components/toast/toast.tsx (TypeScript)

Code before:
              {!revealContentToScreenReader && message !== undefined && this.renderToastMessage('oldMessage', 'true')}
              {revealContentToScreenReader && header !== undefined && this.renderHeader('header')}
              {revealContentToScreenReader && message !== undefined && this.renderToastMessage('header')}
            </div>

            {this.renderButtons(endButtons, 'end')}
          </div>
        </div>
      </Host>
    );
  }
}

const buttonClass = (button: ToastButton): CssClassMap => {
  return {
    'toast-button': true,
    'toast-button-icon-only': button.icon !== undefined && button.text === undefined,
    [`toast-button-${button.role}`]: button.role !== undefined,
    'ion-focusable': true,
    'ion-activatable': true,
    ...getClassMap(button.cssClass),
  };
};

const buttonPart = (button: ToastButton): string => {
  return isCancel(button.role) ? 'button-cancel' : 'button';
};

type ToastPresentOptions = ToastPosition;
type ToastDismissOptions = ToastPosition;


Review comment (suggestion):
```suggestion
  return isCancel(button.role) ? 'button cancel' : 'button';
```

For parts with state/modifiers, we should add it as multiple parts. This way the cancel button still matches the `button` part in addition to `button cancel`.

Code after:
                announces an update to the live region. We do this after
                the toast is fully visible to avoid jank during the presenting
                animation.

                The "key" attribute is used here to force Stencil to render
                new nodes and not re-use nodes. Otherwise, NVDA would not
                detect any changes to the live region.

                The "old" content is hidden using aria-hidden otherwise
                VoiceOver will announce the toast content twice when presenting.
              */}
              {!revealContentToScreenReader && header !== undefined && this.renderHeader('oldHeader', 'true')}
              {!revealContentToScreenReader && message !== undefined && this.renderToastMessage('oldMessage', 'true')}
              {revealContentToScreenReader && header !== undefined && this.renderHeader('header')}
              {revealContentToScreenReader && message !== undefined && this.renderToastMessage('header')}
            </div>

            {this.renderButtons(endButtons, 'end')}
          </div>
        </div>
      </Host>
    );
  }
}

const buttonClass = (button: ToastButton): CssClassMap => {
  return {
    'toast-button': true,
    'toast-button-icon-only': button.icon !== undefined && button.text === undefined,
    [`toast-button-${button.role}`]: button.role !== undefined,
    'ion-focusable': true,
    'ion-activatable': true,
    ...getClassMap(button.cssClass),
  };
};

const buttonPart = (button: ToastButton): string => {
  return isCancel(button.role) ? 'button cancel' : 'button';
};

type ToastPresentOptions = ToastPosition;
type ToastDismissOptions = ToastPosition;
Repository: ionic-team/ionic-framework
File: core/src/utils/forms/compare-with-utils.ts (TypeScript)

Code before:
type CompareFn = (currentValue: any, compareValue: any) => boolean;

/**
 * Uses the compareWith param to compare two values to determine if they are equal.
 *
 * @param currentValue A current value of the control.
 * @param compareValue The value to compare against.
 * @param compareWith The function or property name to use to compare values.
 */
export const compareOptions = (
  currentValue: any,
  compareValue: any,
  compareWith?: string | CompareFn | null
): boolean => {
  if (typeof compareWith === 'function') {
    return compareWith(currentValue, compareValue);
  } else if (typeof compareWith === 'string') {
    return currentValue[compareWith] === compareValue[compareWith];
  } else {
    return Array.isArray(compareValue) ? compareValue.includes(currentValue) : currentValue === compareValue;
  }
};

/**
 * Compares a value against the current value(s) to determine if it is selected.
 *
 * @param currentValue The current value of the control.
 * @param compareValue The value to compare against.
 * @param compareWith The function or property name to use to compare values.
 */
export const isOptionSelected = (

Review comment (suggestion):
```suggestion
 * @param currentValue The current value of the control.
```

Nit: We say "The current" for the function below

Code after:
type CompareFn = (currentValue: any, compareValue: any) => boolean;

/**
 * Uses the compareWith param to compare two values to determine if they are equal.
 *
 * @param currentValue The current value of the control.
 * @param compareValue The value to compare against.
 * @param compareWith The function or property name to use to compare values.
 */
export const compareOptions = (
  currentValue: any,
  compareValue: any,
  compareWith?: string | CompareFn | null
): boolean => {
  if (typeof compareWith === 'function') {
    return compareWith(currentValue, compareValue);
  } else if (typeof compareWith === 'string') {
    return currentValue[compareWith] === compareValue[compareWith];
  } else {
    return Array.isArray(compareValue) ? compareValue.includes(currentValue) : currentValue === compareValue;
  }
};

/**
 * Compares a value against the current value(s) to determine if it is selected.
 *
 * @param currentValue The current value of the control.
 * @param compareValue The value to compare against.
 * @param compareWith The function or property name to use to compare values.
 */
export const isOptionSelected = (
Repository: ionic-team/ionic-framework
File: core/src/components/modal/animations/md.leave.ts (TypeScript)

Code before:
    .duration(200)
    .addAnimation([backdropAnimation, wrapperAnimation])
    .beforeAddWrite(() => {
      if (scrollAtEdge) {
        // Scroll can only be done when the modal is fully expanded.
        return;
      }

      /**
       * If scrollAtEdge is disabled, we need to swap
       * the visibility to the original, so the footer
       *  dismisses with the modal and doesn't stay
       *  until the modal is removed from the DOM.
       */
      const ionFooter = baseEl.querySelector('ion-footer');
      if (ionFooter) {
        const clonedFooter = baseEl.shadowRoot!.querySelector('ion-footer')!;

        ionFooter.style.removeProperty('display');
        ionFooter.removeAttribute('aria-hidden');

        clonedFooter.style.setProperty('display', 'none');
        clonedFooter.setAttribute('aria-hidden', 'true');

        const page = baseEl.querySelector('.ion-page') as HTMLElement;
        page.style.setProperty('padding-bottom', `0`);
      }
    });

  return baseAnimation;
};


Review comment (suggestion):
```suggestion
        page.style.removeProperty('padding-bottom');
```
Since we added the inline padding bottom, then we can safely remove it so it reverts to the original inline styles.

Code after:
    .duration(200)
    .addAnimation([backdropAnimation, wrapperAnimation])
    .beforeAddWrite(() => {
      if (expandToScroll) {
        // Scroll can only be done when the modal is fully expanded.
        return;
      }

      /**
       * If expandToScroll is disabled, we need to swap
       * the visibility to the original, so the footer
       * dismisses with the modal and doesn't stay
       * until the modal is removed from the DOM.
       */
      const ionFooter = baseEl.querySelector('ion-footer');
      if (ionFooter) {
        const clonedFooter = baseEl.shadowRoot!.querySelector('ion-footer')!;

        ionFooter.style.removeProperty('display');
        ionFooter.removeAttribute('aria-hidden');

        clonedFooter.style.setProperty('display', 'none');
        clonedFooter.setAttribute('aria-hidden', 'true');

        const page = baseEl.querySelector('.ion-page') as HTMLElement;
        page.style.removeProperty('padding-bottom');
      }
    });

  return baseAnimation;
};
Repository: ionic-team/ionic-framework
File: core/src/components/button/test/spinner/button.e2e.ts (TypeScript)

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

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

      await page.setIonViewport();

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


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

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

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

      await page.setIonViewport();

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