Skip to content

docs(ui5-side-navigation-item): update documentation #11659

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

Merged
merged 8 commits into from
Jul 3, 2025
Merged

Conversation

GerganaKremenska
Copy link
Member

@GerganaKremenska GerganaKremenska commented Jun 4, 2025

Document the correct usage recommendations for external link items: use the target=_blank attribute along with the href property.

JIRA: BGSOFUIRODOPI-3487

@GerganaKremenska GerganaKremenska requested a review from Copilot June 4, 2025 13:07
@GerganaKremenska GerganaKremenska marked this pull request as draft June 4, 2025 13:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the JSDoc comments for SideNavigationSelectableItemBase to clarify how href, target, selection, and child items should interact.

  • Refined description of the selected property and its relation to href/target="_blank".
  • Added guidance on children usage when href and target are set.
  • Clarified recommended target usage for internal navigation.
Comments suppressed due to low confidence (2)

packages/fiori/src/SideNavigationSelectableItemBase.ts:62

  • [nitpick] Consider rephrasing for clarity, e.g., "Items with an href and target="_blank" are not selectable." to make the statement more concise and consistent with other JSDoc entries.
* Items that have a set href and target set to <code>_blank</code> should not be selectable.

packages/fiori/src/SideNavigationSelectableItemBase.ts:96

  • [nitpick] This sentence is a bit unclear. You could simplify it to: "Items with an href should not specify a target for internal navigation." to better convey the intended restriction.
* **Items that have a set href, should not use target for internal navigation.**

@github-actions github-actions bot added the Stale label Jun 26, 2025
@TeodorTaushanov TeodorTaushanov marked this pull request as ready for review June 30, 2025 12:04
@TeodorTaushanov TeodorTaushanov self-requested a review June 30, 2025 12:04
@TeodorTaushanov TeodorTaushanov requested review from Stoev and a team June 30, 2025 13:24
Copy link

@Stoev Stoev left a comment

Choose a reason for hiding this comment

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

Please change the header to:

docs(ui5-side-navigation-item): update documentation

Please consider changing the additional explanation below the header:

Document the correct usage recommendations for external link items: use the target=_blank attribute along with the href property.

@TeodorTaushanov TeodorTaushanov changed the title docs(ui5-side-navigation-item): updated documentation docs(ui5-side-navigation-item): update documentation Jul 2, 2025
@TeodorTaushanov
Copy link
Member

Please change the header to:

docs(ui5-side-navigation-item): update documentation

Please consider changing the additional explanation below the header:

Document the correct usage recommendations for external link items: use the target=_blank attribute along with the href property.

Done

@TeodorTaushanov TeodorTaushanov requested a review from Stoev July 2, 2025 12:17
Copy link

@Stoev Stoev left a comment

Choose a reason for hiding this comment

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

Looks good, thank you.

@TeodorTaushanov TeodorTaushanov merged commit 5a9ebdf into main Jul 3, 2025
19 of 20 checks passed
@TeodorTaushanov TeodorTaushanov deleted the sideNAv branch July 3, 2025 10:52
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.

4 participants