Upgrade Octicons v18.3.0 #509
Conversation
|
A note, I've also modified some classes in After tracing it back to the PR that added this, and talking to @savetheclocktower it seems that package used So I've updated the code there to use |
|
A reminder to myself, if nothing else, that we need to update https://pulsar-edit.dev/docs/launch-manual/sections/core-hacking/#iconography if/when this PR is merged. |
I think this is absolutely the right approach, we if we depend on something like this it makes no sense to "hack it" to our own specification vs editing our own packages. I do see the change in the So instead of having the static folder icon we could instead do this depending on whether it is open or closed: The outlined one doesn't seem to have a counterpart like the filled one though. One thing I did notice pretty quickly is that some of the icons seem to be just that little bit larger (or at least taller) than the old set. I don't know if this is just me being picky but they seem a little unbalanced, particularly noticeable on the
I wonder if this needs a little scaling? Otherwise if we are after community opinions on aesthetics we could always open things up to a survey. Don't get me wrong, I think this is a really good and important change but we need to get it right as aesthetics are so important to many people. |
|
I'm not sure how good I'll be at editing the scaling, the only thing I can think of, is the new SVGs did come in variations of 12px size, 14px, 16px and so on. So it may be possible that book was selected at a higher size than the others? Although they are SVGs so not sure how much of a difference it makes. I'll take a look, and if there's any others you notice a shout on Discord would be appreciated to take a look at just in case. But otherwise you make super good points about editing our packages for what fits best, and I think a big (visual) PR to |
This is kind of what I was thinking as it should be fairly easy but that does add overhead for the future maintenance. Honestly not sure as that was just my first impressions, we might be able to do something else like vertically centring the text rather than have it anchored to the top. Would need to do some investigating there. |
|
So investigating this @Daeraxa, and I think I've found the root cause of this issue. Essentially within a font file you are able to define a few things that define the sizing of the font:
For each of this values (with the exception of Each So for Meanwhile, as you might expect our brand new font So this does mean, we may have to manually match sizing for previously supported glyphs within the new font file to avoid any kind of substantial visual change. The largest unfortunate fact here is, we can't match any new icons to anything previously. Now we could decide that moving forward we should always plan to use the glyphs at their proper So what I'd really propose doing, is of course matching the sizing of all previously supported icons. Marking down these size changes in the This does add significantly more headache with keeping icons updating, but hopefully the next time we do this, we just decide to use the SVGs rather than font files lol. But what do we think of this plan? |
|
It seems like it'd be a problem for tree-view — a new icon would seem slightly bigger than an icon that had a v4 analog. |
|
An alternative solution here, is we just support more sizing classes. As of right now I believe we have two sizing classes for fonts. So the other possible fix is allow more sizing options so that icons can be made to be an appropriate size where needed. But that would be a lot more work that I'd hope as a reaction to this PR. |
confused-Techie
force-pushed
the
octicons-v18.3.0
branch
from
0097407
to
341f33f
Compare
|
Alright this PR is now ready to merge. I've made all the needed changes on |
|
I do want to quickly mention, I do see the issue with some icons being slightly offcenter. Such as the |
Co-authored-by: Daeraxa <[email protected]>
|
Really sorry it has taken me so long to get around to this, the last few weeks have been really busy. Lots of stuff we have already gone over before but I'm trying to remember where we got with everything... Either way these are some of the observations I've made and I think need to be improved - for right at this moment this isn't offering any solution, just highlighting the issues for discussion at a minimum. Repo/project icon changes on openCurrently a folded project shows the
We should make sure that the SizesThis is something I mentioned before but I think is something we need to make a decision on. From what I can see there is no change on the actual height of the parent item, only the size of the icon displayed within it. The new folder icon is probably the most obvious thing here as it is quite a lot larger than the original:
Icon "mismatches"So this one I'm struggling with, I know we found that the new Octicons set doesn't have some of the icons we used before or at least presents a version of it which is maybe less desirable. For example in the file tree in this PR, Pulsar is using the old Octicons An example of where this sticks out is here:
Obviously we have the ability to use the new icon but I believe we also discussed that as we thought it looked very odd to have a document denoted by a blank page. I don't know a way around this one, no option seems perfect: @confused-Techie you mentioned you were going to play with the sizing classes and/or matching the sizes. Did you have a look at this at the time or did we have to give this up? I'll take another, better, look at this tomorrow as its getting late but just wanted to get something mentioned so I can at least have this on my radar again. |
The way this if else is made, is structured quite weirdly. But leaving it as is, to avoid complexity. We now as only a last resort if no other changes are applied, will swap to the open directory icon, this ensures we don't swap a book or repo icon with the open directory
|
@Daeraxa I really appreciate you taking the time and pointing out these issues here. I've done what I can to make some changes, but before going to far with it, wanted to see what you thought. Folder Icon changing on OpenSo this was a software bug, essentially when the folder icon is triggered for a refresh, that's when we start to inspect it's properties, to apply either the repo icon, folder icon, symlink icon, or now folder open icon. I set the folder set icon to high, and it would be applied before all other icons had a chance. I've placing it's swap at the end, so that everything else has a chance to be used first. SizesThere's a myriad of issues here, essentially some icons have changed shape so much, the size will always be different. Others now just have different dimensions that may mean they need to be scaled, or just cannot fit into the original size. But a select few others just had extra whitespace causing them to be slightly different sizes. Icon MismatchesWhat we were seeing here, really seems to be that with icons being left aligned, now that some icons had left aligned details, it caused the "main" portion of the icon to feel out of place, there's no real way to address this, other than attempting to get the "main" part of the icon to feel naturally aligned. Which I've attempted to do. What I Actually ChangedThe changes here are low, and unique. Since essentially each icon required testing, and trial and error to find what would feel right to get sizing to match up. With that in mind, I'll detail what I've done below, but do want to get some things out of the way:
As you can see these modifications do require lots of manual changes, but I hope this has modified the biggest offenders, and that from here if there's only a few more we can absolutely ensure to take a look at them, but otherwise let me know what you think of how things look now. For some examples of the after from these changes:
|
|
As a slight note to self, and to keep a good history of developments here. In the goal to keep iconography as consistent as possible we are going to be doing two things:
Otherwise in case the raw SVG is ever needed elsewhere for the `updated-file`<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg
width="24"
height="24"
viewBox="0 0 24 24"
version="1.1"
id="svg4"
sodipodi:docname="file-24p.svg"
inkscape:version="1.2.2 (b0a8486, 2022-12-01)"
inkscape:export-filename="file-24p.png"
inkscape:export-xdpi="96"
inkscape:export-ydpi="96"
xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
xmlns:sodipodi="http://sodipodi.sourceforge.net/DTD/sodipodi-0.dtd"
xmlns="http://www.w3.org/2000/svg"
xmlns:svg="http://www.w3.org/2000/svg">
<defs
id="defs8" />
<sodipodi:namedview
id="namedview6"
pagecolor="#ffffff"
bordercolor="#666666"
borderopacity="1.0"
inkscape:showpageshadow="2"
inkscape:pageopacity="0.0"
inkscape:pagecheckerboard="0"
inkscape:deskcolor="#d1d1d1"
showgrid="true"
inkscape:zoom="32"
inkscape:cx="-2.4375"
inkscape:cy="13"
inkscape:window-width="3440"
inkscape:window-height="1372"
inkscape:window-x="1920"
inkscape:window-y="0"
inkscape:window-maximized="1"
inkscape:current-layer="svg4">
<inkscape:grid
type="xygrid"
id="grid19760"
snapvisiblegridlinesonly="false"
empspacing="2.5" />
</sodipodi:namedview>
<path
d="M3 3a2 2 0 0 1 2-2h9.982a2 2 0 0 1 1.414.586l4.018 4.018A2 2 0 0 1 21 7.018V21a2 2 0 0 1-2 2H5a2 2 0 0 1-2-2Zm2-.5a.5.5 0 0 0-.5.5v18a.5.5 0 0 0 .5.5h14a.5.5 0 0 0 .5-.5V8.5h-4a2 2 0 0 1-2-2v-4Zm10 0v4a.5.5 0 0 0 .5.5h4a.5.5 0 0 0-.146-.336l-4.018-4.018A.5.5 0 0 0 15 2.5Z"
id="path2" />
<path
style="fill:none;stroke:#000000;stroke-width:1.45906;stroke-linecap:round;stroke-linejoin:miter;stroke-dasharray:none;stroke-opacity:1"
d="M 6.7004523,6 C 14.265719,6 14.265719,6 14.265719,6"
id="path186" />
<path
style="fill:none;stroke:#000000;stroke-width:1.5;stroke-linecap:round;stroke-linejoin:miter;stroke-dasharray:none;stroke-opacity:1"
d="m 6.775437,11 c 10.453679,0 10.453679,0 10.453679,0"
id="path186-5" />
<path
style="fill:none;stroke:#000000;stroke-width:1.50024;stroke-linecap:round;stroke-linejoin:miter;stroke-dasharray:none;stroke-opacity:1"
d="M 6.7503959,14 C 17.242185,14 17.242185,14 17.242185,14"
id="path186-5-3" />
</svg> |
|
Provided we were done with the iconography changes, I think this PR may be a quick-win. However, if we weren't done with the iconography changes, well... |
|
@Spiker985 Unfortunately this one is still in progress, and quite outdated progress at that. I've converted this to draft to reflect this, but may have to totally revisit this in the future. |
As the title says, in short, this PR upgrades our included octicons from v4.4.0 to v18.3.0.
This PR ensures backwards compatibility is kept with our previous icons.
Additionally, this PR turned out to be much more involved than I originally hoped, since octicons no longer ships a font file, so that had to be created by hand.
As for the short version,
octicons-font-4README.mdto document how this was done, to hopefully help the next person that needs to do this.style-guidepackage to include all new icons.Now for some of the longer details.
For easy reference, on this bump of octicons, the following icons are no longer supported, so will always fallback to
v4.4.0No Longer Supported Icons
As for the new icons we have gained, here is a full list, but you can also refer to any number of the diffs in this document:
New Icons we have gained
Finally feel free to ask any questions about this process, as unfortunately, most of the work being hidden behind a
.wofffile makes this much less obvious than I'd like, but I'm hoping the readme can clear that up a bit.But the last thing I'll do is add a few images.
Octicons 4 on the Left; Octicons 18 on the Right
Octicons 18 on the Right; Octicons 4 on the Left
The need of doing this at all was born out of the discussion Octicons replacement
Lastly, I know as people have already mentioned on Discord, the switch for some icons from filled, to not filled may be controversial.
But I do want to mention, any icons that are changed here, not added, are the exact same name. As in they are the same icon, that may now have a filled counterpart. I personally think changing them up makes Pulsar feel more modern, and achieves the goal of actually updating the iconography.
If people don't like the new icons I'd recommend we instead update what icons are used in these packages, rather than manually editing the icon file to be inaccurate, and dishonest as to what icon is being used. I'd really like to be able to just point people to
octiconsdocks, and say use whatever you find there, they will be the same. Rather than say, look at their docks, but we manually replaced a few since we didn't like them. We don't wanna follow Atom's lead and make everything just slightly off lol.As for the major grip people have brought up of
tree-viewiconography, I personally love the non-filled icons. But if we want I'd recommend making it configurable (surprise coming from me I know lol), but give people the choice, with the default being new icons, since that's the whole point of updating Pulsar, is the new stuff, But I digress.Tree View Changes
As we had discussed and decided, the changes within this PR should include those that relate to getting
tree-viewsynced with these new classes, to ensure everything still looks good once merged.So now that
tree-viewis merged I've gone ahead and made some small changes there, both to keep our icons working as expected, while adding a few new features related to this.READMEfile, I've extended this behavior toCODE-OF-CONDUCTfiles as well, giving them the Octiconscode-of-conducticon.Lastly, and really the biggest change within
tree-viewas I discussed on Discord, we have been usingfs-plusfortree-viewto determine what 'kind' of file every file was. And while this works, the list was rather small, containing only about a dozen extensions to try and cover all binary, markdown, executable, and image files. While I didn't think we would want to expand our maintenance for a new package, to increase this, I've opted to start beginning to use a package I've created on NPMfinden-filum. The advantages of using this new package, means now we are able to accurately distinguish the 'kind' of file a file is based on the extension for just under 1600 different file extensions.But don't worry, the lookup time for all of these was the major focus of my package, with the last following measured stats:
So this shouldn't cause any noticeable performance impact, while being able to accurately report the icons that should be used on a significantly larger set of file types.
Lastly, for our
tree-viewchanges here are some examples: