Menu item types for IModelApp.uiAdmin.showContextMenu should allow React.ReactNode icon types
#521
Comments
|
AppUI-Abstract package is forbidden to have any React dependency, the idea being that if you write for iTwin.js, any UI should be able to render your content, and this UI may not be React. So, while it does work to cast as any, it is because of an implementation detail that allows you to do so, and your code will only work if you are using AppUI. (iTwin.js and AppUI are 2 different things, AppUI-Abstract is misnamed in that sense.) So, you have 2 options, write "iTwin.js" code, which is UI agnostic, or write "AppUI" code, in which case your code can depend on AppUI packages directly. (We are slowly moving more and more of the content of AppUI-Abstract to AppUI react because it became apparent that it is unlikely that other UI will be created, but that was the intent. As such, things like UiItemsProvider now exists in AppUI-React, and do support React icons. (for toolbar, as an example) However, we dont have currently a "ShowContextMenu" API that would do what you need, (however, we could improve |
|
@raplemie thanks for the explanation.
Did i understood correctly that
Not sure what you mean by "used on an object", we are using it to show the context menu for elements in the iModel viewer.
Honestly, my preference would be to use |
As I mention in the other thread, this basically expects svg-loader import value to be provided, I think you could quite easily transform your
Yes, except if you dont provide it direct
At the moment, it is public, but since it offer no additional benefit than using
It would indeed make your code more portable, I strongly suggest writing an adapter function to convert from |
|
Hmm, apparently the documentation is wrong, the svg-loader object is not what is expected, we already support function toDataBase64(dataRaw: string) {
const dataParts = dataRaw.split(",");
const b64 = btoa(dataParts[1]);
return `${dataParts[0]};base64,${b64}`;
} |
|
Just created a PR that will fix the |
I was a bit confused by the documentation, since the Icon component wraps the svg in
That is great news, thank you! |
|
Hi @ImVeryLost, a fix for Icons to support |
|
@raplemie sadly that did not fix the issue completely. Looking at the tests, seems like the major difference is using When an icon gets to IconComponent sanitize it looks like: And after sanitization it looks like: I tried to update the imported svg so the But maybe its my own fault for trying to use a fancy svg - tried with a couple simplier icons (without |
|
From that example, it seems that the datauris are containing double quotes, this is why it's not working and why I use It also contains For some reason, they are escaping That appears to be a bug to me, as it does not follow the RFC... Also, from esbuild very limited example, it seemed like it was actually encoding:
|
|
This is what I get when I use the data uri provided above:
Which styles are not correct ? ( This is the code I used, I manually split the data to simplify the code snippet. icon: IconSpecUtilities.createWebComponentIconSpec(
`data:image/svg+xml,${encodeURIComponent(
decodeURIComponent(
`<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16"><defs><style>.a{fill:url(%23a)}.b{fill:%23000}.c{fill:%23fff}</style><linearGradient id="a" x1="-92.852" y1="-152.013" x2="-107.8787" y2="-167.0397" gradientTransform="translate(-92.3119 -151.4729) rotate(180)" gradientUnits="userSpaceOnUse"><stop offset="0" stop-color="%23ffffff" /><stop offset="0.2178" stop-color="%23000000" /><stop offset="0.5306" stop-color="%23888888" /><stop offset="0.8993" stop-color="%23555555" /><stop offset="0.9947" stop-color="%23000000" /></linearGradient></defs><rect class="a" width="16" height="16" rx="1.746" ry="1.746" /><path class="b" d="M7,1v6H1v2h6v6h2V9h6V7H9V1H7z" /><path class="c" d="M7,1v6H1v2h6v6h2V9h6V7H9V1H7z" /></svg>`
)
)}`
), |
Fair enough, we will investigate what can be done there. If it does not follow standards, I don't expect you to add custom handling.
You are absolutely right, using Also, ignore that the icon itself does not make much sense visually, I changed up the colors and paths since I'm not sure if the original is open sourced... |
|
Looks like esbuild removed the encoding on purpose... |
|
@raplemie added a custom esbuild plugin to our application that will just encode the svg to base64. Tested with those changes & everything works as expected. Thank you for looking into this, |
|
I'll close for now, we'll see what esbuild answer is. |
Not certain if this should be filed in itwinjs backlog or here, since its technically in appui-abstract package.
Is your feature request related to a problem? Please describe.
Menu items in
IModelApp.uiAdmin.showContextMenuare ofAbstractMenuItemPropstype.AbstractMenuItemProps.item.iconhas the typestring | ConditionalStringValue.This icon is shown on the left side of the menu item.
However if you look at the implementation code that actually renders the menu item icon, you find that its rendering it as a
Iconcomponent:Which in turn supports icons of the following types:
So the icon could already be a React.ReactNode. In fact, if you pass a react component as the menu item icon, it does render correctly, you just need to cast it to
anyfirst, which is not ideal.Describe the solution you'd like
CommonItemProps.iconorAbstractMenuItemProps.item.icontype would beIconSpecrather thanstring | ConditionalStringValue.It already supports all
IconSpectypes due to underlying use ofIconComponent, but does not explicitly allow using them.Describe alternatives you've considered
Current workaround is to cast to
any, which is a bit messy.Additional context
Using
IconSpecUtilities.createWebComponentIconSpecdoes not currently work for our use case. Our SVG is inlined using esbuild, and ends up looking likeThis then gets sanitized by the
IconComponenttill its missing half of its data, and thus does not render correctly (or at all)Thats related to #421
The text was updated successfully, but these errors were encountered: