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(#1368,#1792,#1772,#1569): fixing link active when router change #1843

Merged
merged 3 commits into from
May 29, 2024

Conversation

vanessatran-ddi
Copy link
Collaborator

@vanessatran-ddi vanessatran-ddi commented May 9, 2024

Scope:

  1. Fixing App Header: not updating current selection when using routerLink #1772
  2. Fixing App Header: Selected item on mobile is not highlighted #1792
  3. Fixing [Bug]: AppHeader Menu isn't active #1386
  4. Fixing Side menu not showing current page correctly when there is sub item #1569

What are changed:

  • AppHeader active link on mobile and tablet, now has the blue background color and white text color like Figma described
  • AppHeaderMenu active link now has the blue background color and white text color like Figma above.
  • Move as much as possible CSS of GoAAppHeader and GoAAppHeaderMenu under components.css to component itself, using ::slottted
  • Add getMatchedLink to make sure if AppHeader and AppHeaderMenu links have the same weight, then no menu should be active. Change the logic to make sure the highest weight item will be active. Add tests.
  • When URL is changed (routerLink) on single page applications, the link class current should be updated. The current link should be correct.
  • AppHeaderMenu should be closed if router is changed.
  • Tab href is fixed in order to work with Angular.

Demo link: https://vanessatran-ddi.github.io/ui-components/#/

Video demo:

app-header-active.mov
app-header-active-web-docs.mov

@ArakTaiRoth
Copy link
Collaborator

ArakTaiRoth commented May 10, 2024

Finished testing, here are my issues I found

  • Doesn't work if you use routerLink instead of href
  • Doesn't work if you use a click event instead of href
  • Doesn't work with AppHeaderMenu (as in, if you select an item inside an AppHeaderMenu, the top level item doesn't show as selected)

@vanessatran-ddi vanessatran-ddi changed the title fix(#1792): selected item in goa-app-header in mobile not highlighted fix(#1792): fixing link active when router change May 14, 2024
@vanessatran-ddi vanessatran-ddi changed the title fix(#1792): fixing link active when router change fix(#1368): fixing link active when router change & mobile link should be active May 14, 2024
@vanessatran-ddi
Copy link
Collaborator Author

Finished testing, here are my issues I found

  • Doesn't work if you use routerLink instead of href
  • Doesn't work if you use a click event instead of href
  • Doesn't work with AppHeaderMenu (as in, if you select an item inside an AppHeaderMenu, the top level item doesn't show as selected)

Hi @ArakTaiRoth , The second one, as we discussed in Slack, will not be able to be solved using the current logic to detect the URL changed.

@lizhuomeng71
Copy link
Collaborator

issue #1386 is verified fixed on react and angular, tested for both href and routerlink, also tested /start-up and /start-up/overview combination. works as expected

@lizhuomeng71
Copy link
Collaborator

lizhuomeng71 commented May 15, 2024

for react,

when there is only one item in GoAAppHeaderMenu. the GoAAppHeaderMenu is showing always active. even when other GoAAppHeader Item is clicked. Had an huddle and demoed the STR with Vanessa

image

@vanessatran-ddi vanessatran-ddi changed the title fix(#1368): fixing link active when router change & mobile link should be active fix(#1368,#1792,#1772,#1569): fixing link active when router change May 27, 2024
@vanessatran-ddi vanessatran-ddi force-pushed the bug/1792 branch 2 times, most recently from e84ce2d to 4561535 Compare May 27, 2024 17:46
@lizhuomeng71
Copy link
Collaborator

issue is verified on react and angular for chrome, firefox and edge

@lizhuomeng71 lizhuomeng71 marked this pull request as ready for review May 27, 2024 20:26
@vanessatran-ddi vanessatran-ddi marked this pull request as draft May 27, 2024 22:05
@lizhuomeng71 lizhuomeng71 marked this pull request as ready for review May 28, 2024 16:09
@lizhuomeng71
Copy link
Collaborator

issue is verified on react and angular for chrome, firefox and edge

@vanessatran-ddi vanessatran-ddi force-pushed the bug/1792 branch 3 times, most recently from 8f68534 to e1036a0 Compare May 28, 2024 21:59
@ArakTaiRoth ArakTaiRoth merged commit de245a7 into GovAlta:alpha May 29, 2024
1 check passed
@tzuge
Copy link
Collaborator

tzuge commented May 29, 2024

🎉 This PR is included in version 1.17.0-alpha.56 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge tzuge added the released on @alpha Released into alpha. label May 29, 2024
@tzuge
Copy link
Collaborator

tzuge commented May 30, 2024

🎉 This PR is included in version 4.17.0-alpha.28 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented Jun 11, 2024

🎉 This PR is included in version 1.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge
Copy link
Collaborator

tzuge commented Jun 11, 2024

🎉 This PR is included in version 4.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tzuge tzuge added the released Released into production. label Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha Released into alpha. released Released into production.
Projects
None yet
5 participants