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

Code before:
    const { el, helperText, errorText, helperTextId, errorTextId } = this;

    if (el.classList.contains('ion-touched') && el.classList.contains('ion-invalid') && errorText) {
      return errorTextId;
    }

    if (helperText) {
      return helperTextId;
    }

    return undefined;
  }

  /**
   * Responsible for rendering helper text and error text.
   * This element should only be rendered if hint text is set.
   */
  private renderHintText() {
    const { helperText, errorText, helperTextId, errorTextId, checked } = this;

    /**
     * undefined and empty string values should
     * be treated as not having helper/error text.
     */
    const hasHelperText = !!helperText;
    const hasErrorText = !!errorText && !checked;
    if (!hasHelperText && !hasErrorText) {
      return;
    }

    return (
      <div class="checkbox-bottom">
        {hasHelperText && (
          <div id={helperTextId} class="helper-text" part="supporting-text helper-text">
            {helperText}
          </div>
        )}
        {hasErrorText && (
          <div id={errorTextId} class="error-text" part="supporting-text error-text">
            {errorText}
          </div>
        )}
      </div>
    );
  }

  render() {
    const {
      color,
      checked,
      disabled,

Review comment (suggestion):
I do not think this is a good change. If the checkbox is checked, it could still be in an error state. It should be up to the developer to change the errorText value when the checkbox is checked, if that's what they want.

Code after:
    const { el, helperText, errorText, helperTextId, errorTextId } = this;

    if (el.classList.contains('ion-touched') && el.classList.contains('ion-invalid') && errorText) {
      return errorTextId;
    }

    if (helperText) {
      return helperTextId;
    }

    return undefined;
  }

  /**
   * Responsible for rendering helper text and error text.
   * This element should only be rendered if hint text is set.
   */
  private renderHintText() {
    const { helperText, errorText, helperTextId, errorTextId } = this;

    /**
     * undefined and empty string values should
     * be treated as not having helper/error text.
     */
    const hasHintText = !!helperText || !!errorText;
    if (!hasHintText) {
      return;
    }

    return (
      <div class="checkbox-bottom">
        <div id={helperTextId} class="helper-text" part="supporting-text helper-text">
          {helperText}
        </div>
        <div id={errorTextId} class="error-text" part="supporting-text error-text">
          {errorText}
        </div>
      </div>
    );
  }

  render() {
    const {
      color,
      checked,
      disabled,
      el,
      getSVGPath,
      indeterminate,
      inheritedAttributes,
      inputId,
Repository: ionic-team/ionic-framework
File: packages/react/test/base/src/App.tsx (TypeScript)

Code before:
  <IonApp>
    <IonReactRouter>
      <IonRouterOutlet>
        <Route path="/" component={Main} />
        <Route path="/overlay-hooks" component={OverlayHooks} />
        <Route path="/overlay-components" component={OverlayComponents} />
        <Route path="/overlay-components/nested-popover" component={IonPopoverNested} />
        <Route
          path="/overlay-components/modal-conditional-sibling"
          component={IonModalConditionalSibling}
        />
        <Route path="/overlay-components/modal-conditional" component={IonModalConditional} />
        <Route
          path="/overlay-components/modal-datetime-button"
          component={IonModalDatetimeButton}
        />
        <Route
          path="/overlay-components/modal-multiple-children"
          component={IonModalMultipleChildren}
        />
        <Route path="/keep-contents-mounted" component={KeepContentsMounted} />
        <Route path="/navigation" component={NavComponent} />
        <Route path="/tabs" component={Tabs} />
        <Route path="/tabs-basic" component={TabsBasic} />
        <Route path="/icons" component={Icons} />
        <Route path="/inputs" component={Inputs} />
      </IonRouterOutlet>
    </IonReactRouter>
  </IonApp>
);

export default App;


Review comment (suggestion):
Can we add this to `Main.tsx`:

```tsx
<IonItem routerLink="/tabs-direct-navigation">
  <IonLabel>Tabs with Direct Navigation</IonLabel>
</IonItem> 
```

Code after:
const App: React.FC = () => (
  <IonApp>
    <IonReactRouter>
      <IonRouterOutlet>
        <Route path="/" component={Main} />
        <Route path="/overlay-hooks" component={OverlayHooks} />
        <Route path="/overlay-components" component={OverlayComponents} />
        <Route path="/overlay-components/nested-popover" component={IonPopoverNested} />
        <Route
          path="/overlay-components/modal-conditional-sibling"
          component={IonModalConditionalSibling}
        />
        <Route path="/overlay-components/modal-conditional" component={IonModalConditional} />
        <Route
          path="/overlay-components/modal-datetime-button"
          component={IonModalDatetimeButton}
        />
        <Route
          path="/overlay-components/modal-multiple-children"
          component={IonModalMultipleChildren}
        />
        <Route path="/keep-contents-mounted" component={KeepContentsMounted} />
        <Route path="/navigation" component={NavComponent} />
        <Route path="/tabs" component={Tabs} />
        <Route path="/tabs-basic" component={TabsBasic} />
        <Route path="/tabs-direct-navigation" component={TabsDirectNavigation} />
        <Route path="/icons" component={Icons} />
        <Route path="/inputs" component={Inputs} />
      </IonRouterOutlet>
    </IonReactRouter>
  </IonApp>
);

export default App;
Repository: ionic-team/ionic-framework
File: packages/react/test/base/src/pages/TabsDirectNavigation.tsx (TypeScript)

Code before:
        <Route path="/tabs-direct-navigation/library" render={() => <LibraryPage />} exact={true} />
        <Route path="/tabs-direct-navigation/search" render={() => <SearchPage />} exact={true} />
      </IonRouterOutlet>

      <IonTabBar slot="bottom" data-testid="tab-bar">
        <IonTabButton tab="home" href="/tabs-direct-navigation/home" data-testid="home-tab">
          <IonIcon icon={homeOutline}></IonIcon>
          <IonLabel>Home</IonLabel>
        </IonTabButton>

        <IonTabButton tab="radio" href="/tabs-direct-navigation/radio" data-testid="radio-tab">
          <IonIcon icon={radioOutline}></IonIcon>
          <IonLabel>Radio</IonLabel>
        </IonTabButton>

        <IonTabButton tab="library" href="/tabs-direct-navigation/library" data-testid="library-tab">
          <IonIcon icon={libraryOutline}></IonIcon>
          <IonLabel>Library</IonLabel>
        </IonTabButton>

        <IonTabButton tab="search" href="/tabs-direct-navigation/search" data-testid="search-tab">
          <IonIcon icon={searchOutline}></IonIcon>
          <IonLabel>Search</IonLabel>
        </IonTabButton>
        </IonTabBar>
      </IonTabs>
  );
};

export default TabsDirectNavigation;


Review comment (suggestion):
```suggestion
      </IonTabBar>
    </IonTabs>
```

Code after:
        <Route path="/tabs-direct-navigation/library" render={() => <LibraryPage />} exact={true} />
        <Route path="/tabs-direct-navigation/search" render={() => <SearchPage />} exact={true} />
      </IonRouterOutlet>

      <IonTabBar slot="bottom" data-testid="tab-bar">
        <IonTabButton tab="home" href="/tabs-direct-navigation/home" data-testid="home-tab">
          <IonIcon icon={homeOutline}></IonIcon>
          <IonLabel>Home</IonLabel>
        </IonTabButton>

        <IonTabButton tab="radio" href="/tabs-direct-navigation/radio" data-testid="radio-tab">
          <IonIcon icon={radioOutline}></IonIcon>
          <IonLabel>Radio</IonLabel>
        </IonTabButton>

        <IonTabButton tab="library" href="/tabs-direct-navigation/library" data-testid="library-tab">
          <IonIcon icon={libraryOutline}></IonIcon>
          <IonLabel>Library</IonLabel>
        </IonTabButton>

        <IonTabButton tab="search" href="/tabs-direct-navigation/search" data-testid="search-tab">
          <IonIcon icon={searchOutline}></IonIcon>
          <IonLabel>Search</IonLabel>
        </IonTabButton>
      </IonTabBar>
    </IonTabs>
  );
};

export default TabsDirectNavigation;
Repository: ionic-team/ionic-framework
File: core/src/utils/transition/index.ts (TypeScript)

Code before:
    return ionPage;
  }
  // idk, return the original element so at least something animates and we don't have a null pointer
  return element;
};

/**
 * Retrieves the ion-header element from a page based on the
 * direction of the transition.
 *
 * @param opts Options for the transition.
 * @returns The ion-header element or null if not found or not in 'ios' mode.
 */
const getIosIonHeader = (opts: TransitionOptions): HTMLElement | null => {
  const enteringEl = opts.enteringEl;
  const leavingEl = opts.leavingEl;
  const direction = opts.direction;
  const mode = opts.mode;

  if (mode !== 'ios') {
    return null;
  }

  const element = direction === 'back' ? leavingEl : enteringEl;

  if (!element) {
    return null;
  }

  return element.querySelector('ion-header');
};

export interface TransitionOptions extends NavOptions {
  progressCallback?: (ani: Animation | undefined) => void;
  baseEl: any;
  enteringEl: HTMLElement;
  leavingEl: HTMLElement | undefined;
}

export interface TransitionResult {
  hasCompleted: boolean;
  animation?: Animation;
}


Review comment (suggestion):
I didn't see the benefit of querying out the header if the mode is `md`. However, we would have to update this in `next` to use `theme` instead of `mode`.

Code after:
  }

  const ionPage = element.querySelector(':scope > .ion-page, :scope > ion-nav, :scope > ion-tabs');
  if (ionPage) {
    return ionPage;
  }
  // idk, return the original element so at least something animates and we don't have a null pointer
  return element;
};

/**
 * Retrieves the ion-header element from a page based on the
 * direction of the transition.
 *
 * @param opts Options for the transition.
 * @returns The ion-header element or null if not found or not in 'ios' mode.
 */
const getIosIonHeader = (opts: TransitionOptions): HTMLElement | null => {
  const enteringEl = opts.enteringEl;
  const leavingEl = opts.leavingEl;
  const direction = opts.direction;
  const mode = opts.mode;

  if (mode !== 'ios') {
    return null;
  }

  const element = direction === 'back' ? leavingEl : enteringEl;

  if (!element) {
    return null;
  }

  return element.querySelector('ion-header');
};

export interface TransitionOptions extends NavOptions {
  progressCallback?: (ani: Animation | undefined) => void;
  baseEl: any;
  enteringEl: HTMLElement;
  leavingEl: HTMLElement | undefined;
}

export interface TransitionResult {
  hasCompleted: boolean;
  animation?: Animation;
}
Repository: ionic-team/ionic-framework
File: core/src/themes/md/dark.tokens.ts (TypeScript)

Code before:
  textColorRgb: '255, 255, 255',

  backgroundColorStep: {
    50: '#1e1e1e',
    100: '#2a2a2a',
    150: '#363636',
    200: '#414141',
    250: '#4d4d4d',
    300: '#595959',
    350: '#656565',
    400: '#717171',
    450: '#7d7d7d',
    500: '#898989',
    550: '#949494',
    600: '#a0a0a0',
    650: '#acacac',
    700: '#b8b8b8',
    750: '#c4c4c4',
    800: '#d0d0d0',
    850: '#dbdbdb',
    900: '#e7e7e7',
    950: '#f3f3f3',
  },

  textColorStep: {
    50: '#f33f33',
    100: '#e7e7e7',
    150: '#dbdbdb',
    200: '#d0d0d0',
    250: '#c4c4c4',
    300: '#b8b8b8',
    350: '#acacac',
    400: '#a0a0a0',
    450: '#949494',
    500: '#898989',
    550: '#7d7d7d',
    600: '#717171',
    650: '#656565',
    700: '#595959',
    750: '#4d4d4d',
    800: '#414141',
    850: '#363636',
    900: '#2a2a2a',
    950: '#1e1e1e',
  },

  components: {
    IonCard: {
      background: '#1e1e1e',
    },
    IonItem: {

Review comment (suggestion):
So this is weird, but I noticed all of the other colors here seem to be a grey color (`#xyxyxy`) and this one doesn't follow that pattern, so I looked at it and this one is red. Was that intentional?

Maybe you meant `#f3f3f3`? That would be grey like the others.

Code after:
  textColorRgb: '255, 255, 255',

  backgroundColorStep: {
    50: '#1e1e1e',
    100: '#2a2a2a',
    150: '#363636',
    200: '#414141',
    250: '#4d4d4d',
    300: '#595959',
    350: '#656565',
    400: '#717171',
    450: '#7d7d7d',
    500: '#898989',
    550: '#949494',
    600: '#a0a0a0',
    650: '#acacac',
    700: '#b8b8b8',
    750: '#c4c4c4',
    800: '#d0d0d0',
    850: '#dbdbdb',
    900: '#e7e7e7',
    950: '#f3f3f3',
  },

  textColorStep: {
    50: '#f3f3f3',
    100: '#e7e7e7',
    150: '#dbdbdb',
    200: '#d0d0d0',
    250: '#c4c4c4',
    300: '#b8b8b8',
    350: '#acacac',
    400: '#a0a0a0',
    450: '#949494',
    500: '#898989',
    550: '#7d7d7d',
    600: '#717171',
    650: '#656565',
    700: '#595959',
    750: '#4d4d4d',
    800: '#414141',
    850: '#363636',
    900: '#2a2a2a',
    950: '#1e1e1e',
  },

  components: {
    IonCard: {
      background: '#1e1e1e',
    },
    IonItem: {
Repository: ionic-team/ionic-framework
File: core/src/components/action-sheet/action-sheet.tsx (TypeScript)

Code before:
    }
    return true;
  }

  /**
   * Get all buttons regardless of role.
   */
  private getButtons(): ActionSheetButton[] {
    return this.buttons.map((b) => {
      return typeof b === 'string' ? { text: b } : b;
    });
  }

  /**
   * Get all radio buttons (buttons with role="radio").
   */
  private getRadioButtons(): ActionSheetButton[] {
    return this.getButtons().filter((b) => b.role === 'radio' && !isCancel(b.role));
  }

  /**
   * Handle radio button selection and update aria-checked state.
   *
   * @param button The radio button that was selected.
   */
  private async selectRadioButton(button: ActionSheetButton) {
    const buttonId = this.getButtonId(button);

    // Set the active radio ID (this will trigger a re-render and update aria-checked)
    this.activeRadioId = buttonId;
  }

  /**
   * Get or generate an ID for a button.
   *
   * @param button The button for which to get the ID.
   * @param index Optional index of the button in the buttons array.
   * @returns The ID of the button.
   */
  private getButtonId(button: ActionSheetButton, index?: number): string {
    if (button.id) {
      return button.id;
    }
    const allButtons = this.getButtons();
    const buttonIndex = index !== undefined ? index : allButtons.indexOf(button);
    return `action-sheet-button-${this.overlayIndex}-${buttonIndex}`;
  }

  private onBackdropTap = () => {
    this.dismiss(undefined, BACKDROP);
  };

Review comment (suggestion):
```suggestion
  private selectRadioButton(button: ActionSheetButton) {
```

Code after:
    }
    return true;
  }

  /**
   * Get all buttons regardless of role.
   */
  private getButtons(): ActionSheetButton[] {
    return this.buttons.map((b) => {
      return typeof b === 'string' ? { text: b } : b;
    });
  }

  /**
   * Get all radio buttons (buttons with role="radio").
   */
  private getRadioButtons(): ActionSheetButton[] {
    return this.getButtons().filter((b) => {
      const role = b.htmlAttributes?.role;
      return role === 'radio' && !isCancel(role);
    });
  }

  /**
   * Handle radio button selection and update aria-checked state.
   *
   * @param button The radio button that was selected.
   */
  private selectRadioButton(button: ActionSheetButton) {
    const buttonId = this.getButtonId(button);

    // Set the active radio ID (this will trigger a re-render and update aria-checked)
    this.activeRadioId = buttonId;
  }

  /**
   * Get or generate an ID for a button.
   *
   * @param button The button for which to get the ID.
   * @param index Optional index of the button in the buttons array.
   * @returns The ID of the button.
   */
  private getButtonId(button: ActionSheetButton, index?: number): string {
    if (button.id) {
      return button.id;
    }
    const allButtons = this.getButtons();
    const buttonIndex = index !== undefined ? index : allButtons.indexOf(button);
    return `action-sheet-button-${this.overlayIndex}-${buttonIndex}`;
  }
Repository: ionic-team/ionic-framework
File: core/src/components/datetime/datetime.tsx (TypeScript)

Code before:

  /**
   * Clean up all listeners except for the overlay
   * listener. This is so that we can re-create the listeners
   * if the datetime has been hidden/presented by a modal or popover.
   */
  private destroyInteractionListeners = () => {
    const { destroyCalendarListener, destroyKeyboardMO } = this;

    if (destroyCalendarListener !== undefined) {
      destroyCalendarListener();
    }

    if (destroyKeyboardMO !== undefined) {
      destroyKeyboardMO();
    }
  };

  private initializeListeners() {
    this.initializeCalendarListener();
    this.initializeKeyboardListeners();
  }

  /**
   * Fallback to ensure the datetime becomes ready even if
   * IntersectionObserver never reports it as intersecting.
   *
   * This is primarily used in environments where the observer
   * might not fire as expected, such as when running under
   * synthetic tests that stub IntersectionObserver.
   */
  private ensureReadyIfVisible = () => {
    if (this.el.classList.contains('datetime-ready')) {
      return;
    }

    const rect = this.el.getBoundingClientRect();
    if (rect.width === 0 || rect.height === 0) {
      return;
    }

    this.initializeListeners();

    writeTask(() => {
      this.el.classList.add('datetime-ready');
    });
  };

  componentDidLoad() {
    const { el, intersectionTrackerRef } = this;


Review comment (refactor):
```suggestion
   * TODO(FW-6931): Remove this fallback upon solving the root cause
   * Fallback to ensure the datetime becomes ready even if
```

Let's link to the follow-up JIRA ticket.

Code after:

  /**
   * Clean up all listeners except for the overlay
   * listener. This is so that we can re-create the listeners
   * if the datetime has been hidden/presented by a modal or popover.
   */
  private destroyInteractionListeners = () => {
    const { destroyCalendarListener, destroyKeyboardMO } = this;

    if (destroyCalendarListener !== undefined) {
      destroyCalendarListener();
    }

    if (destroyKeyboardMO !== undefined) {
      destroyKeyboardMO();
    }
  };

  private initializeListeners() {
    this.initializeCalendarListener();
    this.initializeKeyboardListeners();
  }

  /**
   * TODO(FW-6931): Remove this fallback upon solving the root cause
   * Fallback to ensure the datetime becomes ready even if
   * IntersectionObserver never reports it as intersecting.
   *
   * This is primarily used in environments where the observer
   * might not fire as expected, such as when running under
   * synthetic tests that stub IntersectionObserver.
   */
  private ensureReadyIfVisible = () => {
    if (this.el.classList.contains('datetime-ready')) {
      return;
    }

    const rect = this.el.getBoundingClientRect();
    if (rect.width === 0 || rect.height === 0) {
      return;
    }

    this.initializeListeners();

    writeTask(() => {
      this.el.classList.add('datetime-ready');
    });
  };

  componentDidLoad() {
    const { el, intersectionTrackerRef } = this;
Repository: ionic-team/ionic-framework
File: core/src/components/radio-group/radio-group.tsx (TypeScript)

Code before:
        ev.preventDefault();
      }
    }
  }

  /** @internal */
  @Method()
  async setFocus() {
    const radioToFocus = this.getRadios().find((r) => r.tabIndex !== -1);
    radioToFocus?.setFocus();
  }

  /**
   * Renders the helper text or error text values
   */
  private renderHintText() {
    const { helperText, errorText, helperTextId, errorTextId, isInvalid } = this;

    const hasHintText = !!helperText || !!errorText;
    if (!hasHintText) {
      return;
    }

    return (
      <div class="radio-group-top" part="top">
        <div id={helperTextId} class="helper-text" part="helper-text" aria-live="polite">
          {!isInvalid ? helperText : null}
        </div>
        <div id={errorTextId} class="error-text" part="error-text" role="alert">
          {isInvalid ? errorText : null}
        </div>
      </div>
    );
  }

  private getHintTextId(): string | undefined {
    const { helperText, errorText, helperTextId, errorTextId, isInvalid } = this;

    if (isInvalid && errorText) {
      return errorTextId;
    }

    if (helperText) {
      return helperTextId;
    }

    return undefined;
  }

  render() {
    const { label, labelId, el, name, value } = this;

Review comment (refactor):
```suggestion
        <div id={helperTextId} class="helper-text" part="supporting-text helper-text" aria-live="polite">
```

Code after:
          this.emitValueChange(ev);
        }

        // Prevent browsers from jumping
        // to the bottom of the screen
        ev.preventDefault();
      }
    }
  }

  /** @internal */
  @Method()
  async setFocus() {
    const radioToFocus = this.getRadios().find((r) => r.tabIndex !== -1);
    radioToFocus?.setFocus();
  }

  /**
   * Renders the helper text or error text values
   */
  private renderHintText() {
    const { helperText, errorText, helperTextId, errorTextId, isInvalid } = this;

    const hasHintText = !!helperText || !!errorText;
    if (!hasHintText) {
      return;
    }

    return (
      <div class="radio-group-top">
        <div id={helperTextId} class="helper-text" part="supporting-text helper-text" aria-live="polite">
          {!isInvalid ? helperText : null}
        </div>
        <div id={errorTextId} class="error-text" part="supporting-text error-text" role="alert">
          {isInvalid ? errorText : null}
        </div>
      </div>
    );
  }

  private getHintTextId(): string | undefined {
    const { helperText, errorText, helperTextId, errorTextId, isInvalid } = this;

    if (isInvalid && errorText) {
      return errorTextId;
    }

    if (helperText) {
      return helperTextId;
    }