ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.93k stars 13.51k forks source link

bug: href property on tab button required for routing despite allowing undefined #22391

Open uncvrd opened 3 years ago

uncvrd commented 3 years ago

Bug Report

Ionic version:

[x] 5.0.7

Current behavior:

I am using an IonTabs project. In my tab bar at the bottom, I have a "Create" button that I would like to have trigger a modal on the screen. However, when I click the tab, I receive a routing error because I am not including the href property in the IonTabButton. The href property isn't marked as required, so I figured I could remove the href property since I don't want to be routed to a different page. Instead I'd like to call the onClick event to trigger the modal on the screen.

Obviously, it's trying to find the value of the href and errors on the split.

Repo to replicate: https://github.com/uncvrd/tabs-as-buttons (I only modified the App.tsx)

Steps to recreate:

  1. Ionic Serve
  2. Click the "Create" tab at the bottom of the page

I would like to be able to use the IonTabButton as a normal button if the href is not added as a property.

Stack trace here:

×
TypeError: Cannot read property 'split' of undefined
IonRouterInner.handleChangeTab
src/ReactRouter/IonRouter.tsx:78
  75 | 
  76 | handleChangeTab(tab: string, path: string, routeOptions?: any) {
  77 |   const routeInfo = this.locationHistory.getCurrentRouteInfoForTab(tab);
> 78 |   const [pathname, search] = path.split('?');
     | ^  79 |   if (routeInfo) {
  80 |     this.incomingRouteParams = { ...routeInfo, routeAction: 'push', routeDirection: 'none' };
  81 |     if (routeInfo.pathname === pathname) {
View compiled
IonTabBarUnwrapped.onTabButtonClick
src/components/navigation/IonTabBar.tsx:178
  175 |       this.props.onIonTabsDidChange(new CustomEvent('ionTabDidChange', { detail: { tab: e.detail.tab } }));
  176 |     }
  177 |     this.setActiveTabOnContext(e.detail.tab);
> 178 |     this.context.changeTab(e.detail.tab, currentHref, e.detail.routeOptions);
      | ^  179 |   }
  180 | }
  181 | 
View compiled
IonTabButton.handleIonTabButtonClick
src/components/navigation/IonTabButton.tsx:23
  20 | 
  21 | handleIonTabButtonClick() {
  22 |   if (this.props.onClick) {
> 23 |     this.props.onClick(new CustomEvent('ionTabButtonClick', {
     | ^  24 |       detail: { tab: this.props.tab, href: this.props.href, routeOptions: this.props.routerOptions }
  25 |     }));
  26 |   }
View compiled
HTMLElement.handler
src/components/utils/attachProps.ts:91
  88 | 
  89 |   // Bind new listener.
  90 |   node.addEventListener(eventName, eventStore[eventName] = function handler(e: Event) {
> 91 |     if (newEventHandler) { newEventHandler.call(this, e); }
  92 |   });
  93 | };
  94 | 
View compiled

Ionic info:

Ionic:

   Ionic CLI       : 6.7.0 (/usr/local/lib/node_modules/@ionic/cli)
   Ionic Framework : @ionic/react 5.4.1

Capacitor:

   Capacitor CLI   : 2.4.2
   @capacitor/core : 2.4.2

Utility:

   cordova-res (update available: 0.15.1) : 0.14.0
   native-run (update available: 1.2.2)   : 1.0.0

System:

   NodeJS : v12.16.2 (/usr/local/bin/node)
   npm    : 6.14.4
   OS     : macOS Catalina
elylucas commented 3 years ago

Could you provide # to the href and have it fit your use case?

I don't think we would want to make the href optional as that would be really confusing.

uncvrd commented 3 years ago

@elylucas thanks for this suggestion! Two things:

  1. I think # is a great idea, however, it would appear that IonTabButton overrides the onClick event? I did this:
<IonTabButton
  tab="create"
  href="#"
  onClick={() => {
    console.log('test');
  }}
>
  <IonIcon icon={addCircle} />
  <IonLabel>Create</IonLabel>
</IonTabButton>

However, I do not see test in my console after clicking on the element, therefore I don't think I can trigger a modal from the onClick.

  1. I understand that making href optional would be very confusing. However, in the typescript typings for IonTabButton, href is already optional. Does this typing need to be updated to prevent someone running in to this issue?

EDIT: so yea it looks like the onClick is overridden on <Host> to call selectTab and then e.preventDefault() is called which prevents event bubbling (I think). https://github.com/ionic-team/ionic-framework/blob/c1455a839a25e3e23b7c9e7b7d930055b24c2ecb/core/src/components/tab-button/tab-button.tsx#L96