Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#2109): tabs query string, loading 1st tab issues #2241

Open
wants to merge 1 commit into
base: alpha
Choose a base branch
from

Conversation

syedszeeshan
Copy link
Collaborator

@syedszeeshan syedszeeshan commented Oct 31, 2024

Before (the change)

After (the change)

-If query string is present, it should be preserved.
-Indicated tab should be honoured.

Make sure that you've checked the boxes below before you submit the PR

  • I have read and followed the setup steps
  • I have created necessary unit tests
  • I have tested the functionality in both React and Angular.

Steps needed to test

Angular playground code

<goa-tabs (_change)="handleTabChange($event)" initialtab="1">
  <goa-tab>
    <div slot="heading">Profile</div>
    <p>
      <b>Profile:</b> Lorem ipsum dolor sit amet, consectetur adipiscing elit,
      sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
    </p>

  </goa-tab>
  <goa-tab>
    <div slot="heading">
      Review pending
      <goa-badge type="important" content="1"></goa-badge>
    </div>
    <p>
      <b>Review pending:</b> Lorem ipsum dolor sit amet, consectetur adipiscing
      elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
    </p>
  </goa-tab>
  <goa-tab>
    <div slot="heading">
      Completed
      <goa-badge type="midtone" content="1"></goa-badge>
    </div>
    <p>
      <b>Completed:</b> Lorem ipsum dolor sit amet, consectetur adipiscing elit,
      sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
    </p>
    <p>New text</p>
  </goa-tab>
  <goa-tab heading="Test 1"> Testing </goa-tab>
</goa-tabs>
import { Component } from "@angular/core";

@Component({
  selector: "abgov-tabs",
  templateUrl: "./tabs.html",
})
export class TabsComponent {

  handleTabChange(event: Event) {
    const customEvent = event as CustomEvent;
    const tabIndex = customEvent.detail.tab;
    console.log(`Tab changed to ${tabIndex}`);
  }
}

@syedszeeshan syedszeeshan force-pushed the SZ-2109-UI branch 2 times, most recently from 03b16ad to fdaf49b Compare November 1, 2024 19:58
@syedszeeshan syedszeeshan changed the title NOT READY - fix(#2109): tabs query string and initial tab issue fix(#2109): tabs query string and initial tab issue Nov 1, 2024
@syedszeeshan syedszeeshan changed the title fix(#2109): tabs query string and initial tab issue fix(#2109): tabs query string, loading 1st tab issues Nov 1, 2024
@lizhuomeng71
Copy link
Collaborator

The function of this issue is verified on react and on angular. One thing we noticed is there are 2 additional line for console.log in the code

@lizhuomeng71 lizhuomeng71 marked this pull request as ready for review November 4, 2024 21:23
@syedszeeshan
Copy link
Collaborator Author

syedszeeshan commented Nov 4, 2024

The function of this issue is verified on react and on angular. One thing we noticed is there are 2 additional line for console.log in the code

I will remove those console.log statements and update the PR shortly.
@lizhuomeng71 PR has been updated.

@syedszeeshan syedszeeshan force-pushed the SZ-2109-UI branch 2 times, most recently from 1025c86 to 7e59286 Compare November 6, 2024 20:18
@syedszeeshan
Copy link
Collaborator Author

syedszeeshan commented Nov 6, 2024

Hi @kevingauld , I have updated this PR and made changes to the getTabNumberFromHash() method. Can you please test again?

I have added playground code. Thare is a commented code section for the 'tabs within tabs scenario', which is not recommended.... therefore code has been commented out.

@lizhuomeng71
Copy link
Collaborator

Hi Team, this issue is verified for react and angular. Tab inside tab is out of scope and not verified, loading first tab, and initial tab is verified

libs/react-components/src/lib/tab/tab.tsx Outdated Show resolved Hide resolved
libs/react-components/src/lib/tab/tab.tsx Outdated Show resolved Hide resolved
libs/web-components/src/components/tabs/Tabs.svelte Outdated Show resolved Hide resolved
libs/web-components/src/components/tabs/Tabs.svelte Outdated Show resolved Hide resolved
@syedszeeshan syedszeeshan force-pushed the SZ-2109-UI branch 2 times, most recently from cc2da93 to 8d8c059 Compare November 22, 2024 14:50
@syedszeeshan syedszeeshan force-pushed the SZ-2109-UI branch 2 times, most recently from 03a30f6 to e5720cc Compare December 3, 2024 04:04
@syedszeeshan
Copy link
Collaborator Author

As discussed in the Dev meeting, we check for the tab indicated in the url (e.g. "/tab-0") one time only.... in the beginning.

@syedszeeshan
Copy link
Collaborator Author

Story-2109-20241203.mp4

vanessatran-ddi
vanessatran-ddi previously approved these changes Dec 4, 2024
vanessatran-ddi
vanessatran-ddi previously approved these changes Dec 4, 2024
@chrisolsen chrisolsen self-requested a review December 6, 2024 16:47
@@ -80,9 +82,11 @@ describe("Tabs", () => {

// last tab
await waitFor(() => {
setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure that the block within the timeout is run when; can you confirm that these tests are run. Because if they are being skipped it is a little bit of a false positive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the setTimeout and PR updated, good thing is the tests passed.

@@ -56,6 +56,7 @@ describe("Tabs", () => {
const result = render(Tabs, { initialtab: 2 });

await waitFor(() => {
setTimeout(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment below as it applies here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the setTimeout and PR updated, good thing is the tests passed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still see the setTimeout


// Find the matching tab based on href
const tabs = _tabsEl?.querySelectorAll('[role="tab"]');
if (!tabs) return null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really needed as the querySelectorAll function will always return either an empty array or one that contains data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR has been updated

Comment on lines 49 to 50
tab.getAttribute("href")?.endsWith(hash) ||
hash.endsWith(tab.getAttribute("href")?.split("#")[1] || "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these if conditions it would be easier to understand if these conditions were extracted out to a descriptive var name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR has been updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tabs: Not preserving the query string when it navigates to the tab anchor
4 participants