iTwin / appui

Monorepo for iTwin.js AppUi
MIT License
8 stars 2 forks source link

Conditional icons fail to render for built in toolbar buttons #900

Closed BeAliAslam closed 1 week ago

BeAliAslam commented 1 week ago

Describe the bug

We are experiencing a bug in AppUI based viewer app that conditional icons are failing to render and only a blank button appears on screen (without icon). The problem is reproducible with built in toggle camera toolbar button available with StandardNavigationToolsUiItemsProvider which is using ToolbarItems.createToggleCameraView to draw the toolbar button.

To Reproduce

No special steps. When application loads up, the vertical toolbar for navigation tools is missing the toggle camera icon.

Expected Behavior

Toggle camera icon should appear on the toolbar button

Screenshots

image

Desktop (please complete the applicable information)

Application is running on windows 11, It is an ElectronJS based app @iTwin/appui-abstract: ^4.7.4 @iTwin/appui-react: ^4.12.0 @iTwin/components-react: ^4.12.0

iTwinJS version: 4.7.4

Additional context

The problem seems to be with conditional icons which are using CondictionalIconItem class of AppUI.

Apparently the class has a function isConditionalIconItem to check if a given item is an instance of this class and it seems to be failing.

The function is using following statement to check the given instance

if (itemPrototype.constructor.name !== "ConditionalIconItem") return false;

From what i observed, the evaluated constructor name of its instances are always prefixed with _ so they are _ConditionalIconItem and not ConditionalIconItem. The ActionItem component uses IconHelper.getIconReactNode which checks if icon is of type CondictionalIconItem using function above and returns null since above function incorrectly returns false.

In getIconReactNode function, if I evaluate the icon using 'icon instanceof ConditionalIconItem', i get true.

The fix is to request the AppUI team to either

Note other similar classes like ConditionalStringValue do not have any such function and all checks are using instanceof throughout AppUI code.

GerardasB commented 1 week ago

Related to #315