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.89k stars 13.52k forks source link

bug: ion-segment and ion-segment-button href not working #20381

Closed yarivgdidi closed 4 years ago

yarivgdidi commented 4 years ago

Bug Report

Ionic version: [x] 5.x

Current behavior: when using the ion-segments with href navigation doesn't work. as a workaround, I wrapped ion-segment-button with <a [routerLink] = "..." and used [routerLinkActive]= "..." in ion-segment-button. This worked fine in V 4 but is now broken I need the links for SEO so (ionChaneg) won't do the trick

Expected behavior: should be able to navigate and determine active segment based on url e.g. https://cubesolver.app/tabs/guide/advanced/pll/f should go to the right page and mark "Advanced", "PLL", and "f" segments as active (the link above is to a working V4

Steps to reproduce: see code example below...

Related code:

<ion-segment [(ngModel)]="segment"  >
  <a [routerLink]="'/tabs/guide/using'" >
    <ion-segment-button  value="using" [routerLinkActive]="'segment-button-checked segment-activated'" >
      <div class="container icon-container">
        <div class="sprite-png sprite-png-custom"></div>
      </div>
      <div class="container text-container">Using</div>
    </ion-segment-button>
  </a>

  <a [routerLink]="'/tabs/guide/solving'">
    <ion-segment-button value="solving"   [routerLinkActive]="'segment-button-checked segment-activated'" >
      <div class="container icon-container">
        <div class="sprite-png sprite-png-solve"></div>
      </div>
      <div class="container text-container">Solving</div>
    </ion-segment-button>
  </a>
  <a [routerLink]="'/tabs/guide/advanced'">
    <ion-segment-button value="f2l"   [routerLinkActive]="'segment-button-checked segment-activated'">
      <div class="container icon-container">
        <div class="sprite-png sprite-png-advanced"></div>
      </div>
      <div class="container text-container">Advanced</div>
    </ion-segment-button>
  </a>
</ion-segment>
insert short code snippets here

Other information:

Ionic info:

insert the output from ionic info here
liamdebeasi commented 4 years ago

Thanks for the issue. Can you clarify the issue you are running into? I am seeing that navigation using routerLink on the site you included in the description.

YarivGdidiNi commented 4 years ago

Hi Ionic 5 uses (ionChaneg) on the ion-segment level. On Ionic 4, I've used [routerLink] and [routerLinkActive], on the ion-segment-button level, to:

  1. Navigate.
  2. Highlight the active segment. I can refactor the app so that navigation will work in ionic 5. but I need the links for SEO purposes (google indexing). And I need to be able to highlight segments if accessed directly via link. e.g. https://cubesolver.app/tabs/guide/advanced/pll/f

Thanks Yariv

liamdebeasi commented 4 years ago

Thanks for the follow up. I understand that the issue lies in the example you have provided, but the steps you have been providing do not reproduce the issue for me.

Here are the steps I am following:

  1. Open https://cubesolver.app/tabs/guide/advanced/pll/f
  2. On the first segment row I observe that the "Advanced" button is selected. The second segment row shows the far right (green square) button selected. The third segment row shows the button with "F" selected. This all appears to match up with what is in the URL.
  3. Clicking "T" in the third segment row updates the URL, changes the view, and selects the "T" segment button. Refreshing the page from there shows the same state.

What is the issue I should be seeing?

yarivgdidi commented 4 years ago

Thanks for the effort what you see is the expected behavior. as I said it works fine on ionic 4 (which is deployed) on the local Dev environment with Ionic5 it is broken

liamdebeasi commented 4 years ago

Can you provide a code reproduction of the issue in a v5 application?

yarivgdidi commented 4 years ago

Hi Liam Sorry for the delay in the response

I've created an example to reproduce the roblem git clone https://github.com/yarivgdidi/ion-segments-20381.git npm i ionic serve

You will see three tabs, each with a slightly different implementation of the segments. In desktop mode all works almost as expected with minor issues. In development mode, mobile emulation you'll see unexpected results.

take tab 3 for example, which is the closest to my app implementation. When clicking on the segment icon (the image, i.e. the inner div), navigation won't work. Clicking outside the icon (in the button area) makes it work again. And many other glitches

in package.json (see also package_orig.json and package_ionic5.json) change versions from ionic 5 to 4

"@ionic-native/core": "^5.19.1",
"@ionic-native/splash-screen": "^5.19.1",
"@ionic-native/status-bar": "^5.19.1",
"@ionic/angular": "^5.0.0-rc.3",

to: "@ionic-native/core": "^5.0.0", "@ionic-native/splash-screen": "^5.0.0", "@ionic-native/status-bar": "^5.0.0", "@ionic/angular": "^4.7.1",

npm i ionic serve

And everything work smooth again.

liamdebeasi commented 4 years ago

What are the "many other glitches"? Please be as specific as you can -- this will help us resolve any issues faster.

yarivgdidi commented 4 years ago

Hi Congrats on the new release :-) After upgrading to 5.0 and minor changes to my code, things looks better.

By other glitches, I meant unexpected behavior where I could't put my finger on what I've did different.

To sum things up: For the reproduction I've made 3 slightly different implementation of segments with Icon in the button tab one - as in documentation tab two - button wrapped with a link (I need this for SEO) tab three - button wrapped with like in tab two but navigation is done from JS on iconChange event

Most issues where when clicking on the inner div (the icon) in mobile simulation mode

Tab three seems to work OK now - main issues were propagation of inner div event causing to bad routing are now solved

Tab two - navigation doesn't work in mobile simulation (as did in Ionic 4 and do in desktop mode)

Tab one - because it is not wrapped with link, when refreshing the page - state is not preserved

Thanks again for the effort I feel safe to upgrade now

Yariv

liamdebeasi commented 4 years ago

Ok thanks for the follow up and additional info! I will dive more into this and will post here if I have any follow up questions for you. 🚀

liamdebeasi commented 4 years ago

Can you try the following dev build and let me know if it resolves the issue where the "squares" do not activate the segment button?

npm i @ionic/angular@5.1.0-dev.202002172232.b798df7

yarivgdidi commented 4 years ago

Hi Liam I also had issues similar to #20466 (Android) on some tabs in my App, while other worked fine. I was able to narrow it to the HTML Template. The segments that were not directly surrounded by ion-toolbar had issues in android. As for this case. As mentioned, I've tried three Implementation in the reproduction app: Tab one - navigation in the code on ionChange event work reasonable . Tab two - navigation through surrounding link (without js code) works well in desktop mode and still not working in mobile emulation. tested also with the fix you've send and is working in ionic 4. Tab three - combination of surrounding link and navigation with JS works well. I was able to narrow issues by adjusting the the HTML to match the documented example

  1. surrounding ion-toolbar.
  2. nesting ion-label.

before that, I had lot's of glitches, i.e. some time it works and some time don't without me able to say the difference
for your convenience, here a link to the reproduction repo: https://github.com/yarivgdidi/ion-segments-20381 and to the full app (now ng9/ionic5 in production): https://cubesolver.app/tabs/home

hope this helps with your investigation and thanks again for the help. Yariv P.S can I look at the diff of the changes you made compare to the main brunch? I could't find a branch / pull request

liamdebeasi commented 4 years ago

@yarivgdidi The PR for this bug is https://github.com/ionic-team/ionic/pull/20522.

It looks like there are 2 bugs. The first is the bug that https://github.com/ionic-team/ionic/pull/20522 fixes, and the second is the bug in https://github.com/ionic-team/ionic/issues/20466.

I am going to merge in the fix for this bug and close out this issue. We can handle the bug for https://github.com/ionic-team/ionic/issues/20466 in the other thread.

liamdebeasi commented 4 years ago

Thanks for the issue. This has been resolved via https://github.com/ionic-team/ionic/pull/20522 and will be available in an upcoming release of Ionic Framework.

ionitron-bot[bot] commented 4 years ago

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.