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

feat(ui5-side-navigation): add external link icon #8199

Merged
merged 4 commits into from
Feb 13, 2024

Conversation

kskondov
Copy link
Contributor

if the navigation item is an external link, show the external link indicator

if the navigation item is an external link, show the external link indicator
@kskondov kskondov requested a review from a team January 30, 2024 12:39
Copy link
Contributor

@LidiyaGeorgieva LidiyaGeorgieva left a comment

Choose a reason for hiding this comment

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

Address comments from code review. Expand icon is no longer missing
Copy link
Contributor

@LidiyaGeorgieva LidiyaGeorgieva left a comment

Choose a reason for hiding this comment

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

Please add more samples:

  • in the Story books
  • in the fixed part of the Side Navigation
  • maybe example with disabled item

Please add tests.

Please check again if the color of the arrow is correct:
https://wiki.one.int.sap/wiki/display/CPDesign/Vertical+Navigation
image
https://wiki.one.int.sap/wiki/pages/viewpage.action?pageId=2650676561
image
currently is "#1d2d3e;" for Horizon

packages/fiori/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/fiori/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/fiori/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/main/src/NavigationMenu.hbs Outdated Show resolved Hide resolved
Copy link
Member

@TeodorTaushanov TeodorTaushanov left a comment

Choose a reason for hiding this comment

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

External links in the overflow menu are not opened by keyboard Enter and Space key

packages/fiori/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/fiori/src/SideNavigation.hbs Outdated Show resolved Hide resolved
packages/fiori/src/SideNavigationItemBase.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@LidiyaGeorgieva LidiyaGeorgieva left a comment

Choose a reason for hiding this comment

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

In SideNavigationOnly.html test page the arrow for expand/collapse is grey now.
image
This is not correct.

Copy link
Contributor

@LidiyaGeorgieva LidiyaGeorgieva left a comment

Choose a reason for hiding this comment

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

+1

@kskondov kskondov merged commit 47e28c5 into main Feb 13, 2024
8 checks passed
@kskondov kskondov deleted the SideNavigationExternalLinks branch February 13, 2024 12:49
tsanislavgatev pushed a commit that referenced this pull request Feb 20, 2024
* feat(ui5-side-navigation): add external link icon

if the navigation item is an external link, show the external link indicator

* feat(ui5-side-navigation): add external link icon

Address comments from code review. Expand icon is no longer missing

* feat(ui5-side-navigation): add external link icon

Address review comments

* feat(ui5-side-navigation): add external link icon

changed samples
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.

3 participants