feat: add support for .svg file paths as Action icon#219
feat: add support for .svg file paths as Action icon#219tlambert03 merged 9 commits intopyapp-kit:mainfrom
.svg file paths as Action icon#219Conversation
dalthviz
changed the title
.svg file paths as action icons
dalthviz
changed the title
.svg file paths as action icons.svg file paths as Action icon
dalthviz
changed the title
.svg file paths as Action icon.svg file paths as Action icon [WIP]
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 31 31
Lines 1875 1878 +3
=========================================
+ Hits 1875 1878 +3 ☔ View full report in Codecov by Sentry. |
dalthviz
changed the title
.svg file paths as Action icon [WIP].svg file paths as Action icon
|
@tlambert03 what do you think of this PR? |
|
I'm out of the country for another few days, sorry for the delay |
no worries at all! Just wanted to ping in case it got forgotten 😊 |
demo/model_app.py
Outdated
| ), | ||
| types.Action( | ||
| id="about", | ||
| icon=f"file://{ABOUT_ICON_PATH}", |
There was a problem hiding this comment.
Hi there, thank you for pushing foward this! Quick comment, I think there is a need (at least on Windows) to put three slashes here (///) otherwise no icon is shown:
There was a problem hiding this comment.
i see, apologies for changing that! I thought the third slash was an oversight (since there's also a preceding slash for posix systems in ABOUT_ICON_PATH).
There was a problem hiding this comment.
Oh I see, maybe we should show over the example the need to handle the slashes for file: depending on the platform? Checking https://en.wikipedia.org/wiki/File_URI_scheme I see that on Unix the preceding slash is taken into account for the three slashes the URI has before the actual file path 🤔
There was a problem hiding this comment.
Based on the fact that your previous version was working fine (and tests are passing again after reverting it), it appears to be fine for posix systems to have four slashes (file:////Users...). So I'm fine leaving it as is and not doing anything platform specific. But I suppose we could add a note somewhere? Your call, I'm fine just merging
There was a problem hiding this comment.
Woah I said "fine" a lot in there 😅
There was a problem hiding this comment.
Over the last commit adffa9b I added some info about the possibility to use a .svg file path over the Action icon field with a note just in case. Seems like an approapiate place but let me know what you think!
|
all set for merge? |
|
Yep 👍 |
3 participants
Closes #217
Related to napari/napari#7133
A preview from the demo modification: