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(package): support icon urls in radial menu #487

Merged
merged 2 commits into from Feb 18, 2024

Conversation

ktx-mega
Copy link
Contributor

@ktx-mega ktx-mega commented Jan 20, 2024

I like the radial menu.
I use it in one of my projects, and it lack the option to use non FA icons.

This PR is an example implementation to add support for this.
I added width and height parameters to adapt to the images.

I also fixed the type in debug/radial.ts.

Documentation update can be found here : #overextended/overextended.github.io#169

@LukeWasTakenn
Copy link
Member

What you are proposing are images though, they aren't icons, they don't scale like SVGs and they tend to be blurry.

Also the ability to change the height and width is just a disaster waiting to happen with someone releasing a resource using the radial menu.

@ktx-mega
Copy link
Contributor Author

In my use case, I generate the radial items dynamically based on the response of an http query. This query includes an image link that is used as an icon. It is not vectorial, but unless very much upscaled it is not blurry. From what I tested I didn't have any blurry results.

The ability to change the height and width is here because the image size and offset might differ in different cases and that allow to fix this. The test image I provided in the PR looks good with a width/height of 35, whereas the images I use in my project looks better with 75.
I don't think this is a disaster waiting to happen since it is managed by the person writing the mod, so they should be responsible when using this feature. In the same way, setting 3 lines long label won't look good either, yet it is not a disaster.

@ktx-mega
Copy link
Contributor Author

Here's what it looks like for me, with a size of 75 :
image

@ktx-mega
Copy link
Contributor Author

Also, this feature is implemented in the context menu, I reused the code to test the icon parameter from there. The only difference is that you can't change the size there.

Copy link
Member

@thelindat thelindat left a comment

Choose a reason for hiding this comment

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

Should be fine? Might be good to limit size though.
Need type changes for ts package and luadocs.

@ktx-mega
Copy link
Contributor Author

ktx-mega commented Feb 2, 2024

Thanks for the feedback.

Sorry about the types, I don't use TS so I forgot the interface, and I'm not familiar with LUA, I didn't see the doc.
I clamped the size between 0 and 100. I'm not a fan of limiting upward because it is an arbitrary value but this should reduce the risks of disasters.

Did I miss anything else ?

@ktx-mega
Copy link
Contributor Author

Up,
Luke, anything else I need to do ?

@LukeWasTakenn LukeWasTakenn merged commit 78891fb into overextended:master Feb 18, 2024
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.

None yet

3 participants