feat(ui5-side-navigation): add actions and unselectable items #10482

dimovpetar · 2025-01-07T13:27:38Z

BGSOFUIRODOPI-3344

…igation_actions_leaves

Stoev

As tentative suggestion please consider changing summary sentence "feat(ui5-side-navigation): add actions and unselectable items #10482" to "feat(ui5-side-navigation): adds actions and unselectable items #10482".

Reason for suggestion proposal is: https://github.com/SAP/openui5/blob/master/docs/guidelines/jsdoc.md - JSDoc Guidelines: "For actions, start directly with an appropriate verb in the third person: Adds, allocates, constructs, converts, deallocates, destroys, gets, provides, reads, removes, represents, returns, sets, saves, and so on".

packages/fiori/src/SideNavigationSelectableItemBase.ts

Stoev · 2025-01-16T07:45:54Z

packages/fiori/src/types/SideNavigationItemDesign.ts

+	Default = "Default",
+
+	/**
+	 * Design for items that perform an action, such as opening a dialog.


Please consider as alternative:
Design for items that trigger an action,

Just a thought: would it be helpful to state that these items should not contain child items?

Yes, sounds better. Please check again

dimovpetar · 2025-01-20T08:14:07Z

As tentative suggestion please consider changing summary sentence "feat(ui5-side-navigation): add actions and unselectable items #10482" to "feat(ui5-side-navigation): adds actions and unselectable items #10482".

Reason for suggestion proposal is: https://github.com/SAP/openui5/blob/master/docs/guidelines/jsdoc.md - JSDoc Guidelines: "For actions, start directly with an appropriate verb in the third person: Adds, allocates, constructs, converts, deallocates, destroys, gets, provides, reads, removes, represents, returns, sets, saves, and so on".

This is the guideline that applies here https://sap.github.io/ui5-webcomponents/docs/contributing/conventions-and-guidelines/#commit-description. It should be in imperative present tense.

packages/fiori/src/SideNavigationSelectableItemBase.ts

alexandar-mitsev · 2025-01-20T08:47:57Z

packages/website/docs/_components_pages/fiori/SideNavigation/SideNavigation.mdx

Update the existing sample to include the following:

all external links must be "unselectable". Also please check the "NavigationLayout" sample and do the same

better to put the external links in the fixed items area, as those are the guidelines

add several parent elements which are not selectable. Or even better, make a new sample where all parents are unselecatable - as the guideline is to use only selectable or only unselectable parent items in the same side nav

done

done

TODO - add new sample

packages/website/docs/_samples/fiori/SideNavigation/QuickAction/main.js

alexandar-mitsev · 2025-01-20T09:02:05Z

packages/fiori/src/SideNavigationItem.ts

+		if (this.unselectable) {
+			this._toggle();
+		}
+


Quick action can not be triggered by pressing "Enter". It is only triggered by click or pressing "Space"
On other hand if you put "unselectable" on an external link it is triggered by "Enter" but not by "Space"
Also add a test for this.

unselectable items are now also activated by Enter.
Please open separate issue that external links are not opened with Space. It is working like that on the master branch too.

packages/fiori/src/SideNavigationItem.ts

packages/fiori/src/SideNavigationSelectableItemBase.ts

packages/fiori/src/SideNavigationItemTemplate.tsx

alexandar-mitsev · 2025-01-23T11:16:49Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+	 *
+	 *
+	 * **Guidelines**:
+	 * - External links should not be unselectable


remove "not". Text should say "External links should be unselectable"

alexandar-mitsev · 2025-01-23T11:18:27Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+	@property()
+	design: `${SideNavigationItemDesign}` = "Default";
+
+	/**


@Stoev please re-check this part of the documentation as it was significantly changed.

alexandar-mitsev · 2025-01-23T11:29:16Z

packages/fiori/src/SideNavigationItem.ts

@@ -201,6 +225,12 @@ class SideNavigationItem extends SideNavigationSelectableItemBase {
 	get isSideNavigationItem() {
 		return true;
 	}
+
+	_toggle() {


In ui5-side-navigation, in collapsed mode - go to a single click area parent, open the menu with children - press enter or space on the first item (this would be the parent itself) - menu closes. Next time when you open the menu - the child items are gone

packages/fiori/src/SideNavigationSelectableItemBase.ts

Stoev

please consider some suggestions.

Stoev · 2025-01-23T13:12:23Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+	 *
+	 * **Guidelines**:
+	 * - External links should not be unselectable
+	 * - Items that trigger actions (with design "Action") should be unselectable


Please place a full stop:
unselectable.

Stoev · 2025-01-23T13:21:57Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+	 * Indicates whether the navigation item is selectable. By default all items are selectable unless specifically marked as unselectable.
+	 *
+	 * When a parent item is marked as unselectable, clicking it will only expand or collapse its sub-items.
+	 * To achieve better user experience you should not mix unselectable parent items and selectable parent items in the same side navigation.


Please consider simplifying, for example:
To improve user experience do not mix unselectable parent items with selectable parent items in a single side navigation.

Stoev · 2025-01-23T13:52:00Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+	design: `${SideNavigationItemDesign}` = "Default";
+
+	/**
+	 * Indicates whether the navigation item is selectable. By default all items are selectable unless specifically marked as unselectable.


Would it add value for the reader if the term here is more accurate: instead of 'navigation item' we use 'ui5-side-navigation-item'?

Stoev · 2025-01-23T13:58:18Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+	/**
+	 * Indicates whether the navigation item is selectable. By default all items are selectable unless specifically marked as unselectable.
+	 *
+	 * When a parent item is marked as unselectable, clicking it will only expand or collapse its sub-items.


Please consider avoiding the verb "click" whenever possible: (https://help.sap.com/docs/SNG_FOR_UA/e33c591ae4494a659a3f5f983c9d1161/80cf1399d10b47b482e833b6dbe9211c.html?state=DRAFT&q=accessability):

When a parent item is marked as unselectable, selecting it will only expand or collapse its sub-items.

Stoev · 2025-01-23T14:51:47Z

packages/fiori/src/SideNavigationSelectableItemBase.ts

+import type SideNavigationItemDesign from "./types/SideNavigationItemDesign.js";
+import type { AccessibilityAttributes } from "@ui5/webcomponents-base/dist/types.js";
+
+type SideNavigationItemAccessibilityAttributes = Pick<AccessibilityAttributes, "hasPopup">;

 /**
 * Fired when the component is activated either with a


Is this sentence incomplete?

feat(ui5-side-navigation): add actions and option to select leaves only

ef74d27

dimovpetar marked this pull request as draft January 7, 2025 13:27

dimovpetar added 10 commits January 13, 2025 10:56

refactor: use unselectable instead of interaction mode

a27775e

test: isSelectable

22257c0

feat: all parts of unselectable items now can toggle them

4adbd21

test: reorder tests

1503d1d

test: proper assertion

d8fc7b7

feat: add ariaAttributes with hasPopup

a9116a3

test: correct indexes

9cdeaa9

docs: annotate new properties

1afccc5

Merge branch 'main' of github.com:SAP/ui5-webcomponents into side_nav…

4f21684

…igation_actions_leaves

fix: export SideNavigationItemAccessibilityAttributes

fb131e1

dimovpetar marked this pull request as ready for review January 14, 2025 08:25

dimovpetar changed the title ~~feat(ui5-side-navigation): add actions and option to select leaves only~~ feat(ui5-side-navigation): add actions and unselectable items Jan 14, 2025

dimovpetar added 5 commits January 14, 2025 13:42

test: add tests for selection-change event

c944047

docs: add sample

6d86a32

fix: toggle unselectable items with keyboard

f926cc8

docs: document restriction not to use subitems with actions

f50bac7

Merge branch 'main' of github.com:SAP/ui5-webcomponents into side_nav…

1112cf5

…igation_actions_leaves

dimovpetar requested review from a team and Stoev January 14, 2025 14:13

Stoev reviewed Jan 16, 2025

View reviewed changes

docs: improve documentation

31fb11a

dimovpetar requested a review from Stoev January 20, 2025 08:14

alexandar-mitsev requested changes Jan 20, 2025

View reviewed changes

alexandar-mitsev reviewed Jan 21, 2025

View reviewed changes

packages/fiori/src/SideNavigationItemTemplate.tsx Outdated Show resolved Hide resolved

dimovpetar added 2 commits January 21, 2025 13:25

fix: properly fire selection-change from in collapsed mode

6477752

fix: forward design attribute to items in collapsed mode

8adc31a

dimovpetar added 7 commits January 21, 2025 16:17

fix: aria-haspopup is now added in collapsed mode

e3e99f1

fix: don't toggle expanded property on leaf nodes

4cc9f56

fix: fire click event on Enter

1d15c74

fix: set role to menuitem for unselectable items

564a6df

docs: improve unselectable description

3865169

docs: make all external links unselectable

216db08

docs: replace alert with ui5-dialog

a4993ef

alexandar-mitsev requested changes Jan 23, 2025

View reviewed changes

Stoev reviewed Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui5-side-navigation): add actions and unselectable items #10482

feat(ui5-side-navigation): add actions and unselectable items #10482

dimovpetar commented Jan 7, 2025

Stoev left a comment

Stoev Jan 16, 2025

dimovpetar Jan 20, 2025

dimovpetar commented Jan 20, 2025

alexandar-mitsev Jan 20, 2025

dimovpetar Jan 21, 2025

alexandar-mitsev Jan 20, 2025

dimovpetar Jan 21, 2025

alexandar-mitsev Jan 23, 2025

alexandar-mitsev Jan 23, 2025

alexandar-mitsev Jan 23, 2025

Stoev left a comment

Stoev Jan 23, 2025

Stoev Jan 23, 2025

Stoev Jan 23, 2025

Stoev Jan 23, 2025

Stoev Jan 23, 2025

feat(ui5-side-navigation): add actions and unselectable items #10482

Are you sure you want to change the base?

feat(ui5-side-navigation): add actions and unselectable items #10482

Conversation

dimovpetar commented Jan 7, 2025

Stoev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimovpetar commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stoev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment