Skip to content

Update inconsistent SVG icons - #14563

Merged
pavithraes merged 12 commits into
branch-3.8from
pavithraes/14541-fix-svg-icons
Aug 7, 2025
Merged

pavithraes merged 12 commits into
branch-3.8from
pavithraes/14541-fix-svg-icons

Conversation

@pavithraes

@pavithraes pavithraes commented Jul 17, 2025 •

Copy link
Copy Markdown
Member

All pull requests must have an associated issue in the issue tracker. If there
isn't one, please go open an issue describing the defect, deficiency or desired
feature. You can read more about our issue and PR processes in the
wiki.

Comment thread bokehjs/src/less/icons/wheel-pan.svg Outdated
@mattpap mattpap added this to the 3.8 milestone Jul 18, 2025
@mattpap

mattpap commented Jul 18, 2025

Copy link
Copy Markdown
Contributor

color_scatter example before and after:

image image

Looks much better.

@mattpap

mattpap commented Jul 18, 2025

Copy link
Copy Markdown
Contributor

Baseline images need to be updated.

@pavithraes
pavithraes marked this pull request as ready for review July 22, 2025 12:57
@mattpap

mattpap commented Jul 23, 2025 •

Copy link
Copy Markdown
Contributor

marked this pull request as ready for review

The label says otherwise.

@mattpap
mattpap self-requested a review July 23, 2025 05:34

@mosc9575 mosc9575 Jul 24, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like the alpha value of the filled areas in some icons is lost when transferring the svg to png.

grafik

The append-mode.sgv as an example:
grafik

Not sure if anything can be done about it.

@mattpap

mattpap commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

@pavithraes, you asked about test failures in yesterday's meeting. Everything is unrelated. Failures on Windows were resolved in PR #9087 (last commit) and bokehjs failures are addressed in PR #14573.

@mattpap

mattpap commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

What happened here?
image
This didn't look like this during my earlier review.

@mattpap

mattpap commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

I suppose commit 0d0a6b9 is responsible.

@pavithraes

Copy link
Copy Markdown
Member Author

Discussed this with Mateusz -- and both the issues seem to be related to masks, and needs further investigation.

@mattpap

mattpap commented Jul 31, 2025 •

Copy link
Copy Markdown
Contributor

What works with the CSS mask approach is this:

<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" stroke-width="2" stroke="currentColor" fill="none" stroke-linecap="round" stroke-linejoin="round">
  <path d="..." fill="currentColor" fill-opacity="0.4" />

i.e. use currentColor instead of any specific colors and then use fill-opacity to control the alpha channel.

I applied this change to all (?) new icons. Any future changes must follow this pattern:

  • keep all stroke related attributes in <svg>
  • set fill related attributes as explained above
  • don't use <g>, clip paths, etc. if not actually needed

All of this finally brings new icons on par with existing SVG icons (in terms of the structure of the SVG).

@mattpap
mattpap force-pushed the pavithraes/14541-fix-svg-icons branch from ea9428f to 2ed1fe9 Compare July 31, 2025 06:44
@mosc9575

mosc9575 commented Jul 31, 2025 •

Copy link
Copy Markdown
Contributor

@mattpap There are three observations I want to mention.

  1. One icon is missing in "bokehjs/test/baselines/linux/Icons__should_support_all_icons_defined_in_less_icons.less.png", I think it is the box-edit.svg. Marked in green.
  2. One icon has a different color. The toggle-mode.svg is maybe not updated. Marked in blue.
  3. One icon appears two times (y-grip.svg). I belive one of these two should be the x-grip.svg. Marked in red.
all_icons

Furthermore some icons defined in https://github.com/bokeh/bokeh/blob/branch-3.8/bokehjs/src/less/icons.less are missing, like fullscreen.svg and trash.svg.

@pavithraes

Copy link
Copy Markdown
Member Author

One icon has a different color. The toggle-mode.svg is maybe not updated. Marked in blue.

Just noting that yes, this was not redesigned. The png version was just converted into svg.

@pavithraes

pavithraes commented Aug 6, 2025 •

Copy link
Copy Markdown
Member Author

@mosc9575 - Thanks for the reviews!

One icon appears two times (y-grip.svg). I belive one of these two should be the x-grip.svg. Marked in red.

Yes, good catch! I can update!

One icon is missing in "bokehjs/test/baselines/linux/Icons__should_support_all_icons_defined_in_less_icons.less.png", I think it is the box-edit.svg. Marked in green.

Indeed, I see on a local test run though, so I can update the baseline once CI completes for x-grip fix. :)

@pavithraes pavithraes mentioned this pull request Aug 6, 2025
1 of 3 tasks
@pavithraes

pavithraes commented Aug 6, 2025 •

Copy link
Copy Markdown
Member Author

Edit: This resolved after merging branch-3.8.

There is a test failure, which I think is unrelated to this PR:

✗ Bug
 └┬─ in issue #14207
  └── has zoom in when visibility changes
timeout

@mattpap

mattpap commented Aug 7, 2025

Copy link
Copy Markdown
Contributor

Docs CI failure is unrelated and fixed in PR #14487 (985259e).

@mattpap mattpap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still don't like how wheel zoom and related icons look look and there are icons (e.g. toggle-mode.svg) that are implemented in an awkward way. However, this is a huge improvement over the original, so let's get this merged and continue refining in future PRs.

@pavithraes

Copy link
Copy Markdown
Member Author

@mattpap and @mosc9575 -- Thank you so much for all your help with this PR! 🌻

@pavithraes
pavithraes merged commit 57a3cdf into branch-3.8 Aug 7, 2025
@mattpap
mattpap deleted the pavithraes/14541-fix-svg-icons branch August 7, 2025 14:06
@github-actions

Copy link
Copy Markdown

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New SVG icons have inconsistent line widths

4 participants