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

[BasicUI] Use inline SVG for "colorless" SVG icons #1799

Merged
merged 3 commits into from
Jul 16, 2023

Conversation

lolodomo
Copy link
Contributor

Related to #1743

Use a HTML svg element (inline SVG) instead of a img element
when the icon servlet returns a SVG icon containing "currentColor".

Then it is possible to adjust the color of these icons by using the
iconcolor sitemapo attribute.

Signed-off-by: Laurent Garnier lg.hc@free.fr

@lolodomo lolodomo added enhancement New feature or request basic ui Basic UI labels Mar 17, 2023
@lolodomo lolodomo requested a review from a team as a code owner March 17, 2023 19:42
@lolodomo
Copy link
Contributor Author

lolodomo commented Mar 17, 2023

This PR is currently based on PR #1792. I will rebase it when #1792 is merged. Edit: done.

The change is working well but I have still to check if cache is really working when using "fectch". It looks like it is not with Firefox.

The disadvantages of this change is that:

  1. an icon is now potentially retrieved twice when a new page is opened
  2. the weight of the page when using inline SVG is bigger than when using img tags
  3. The browser cache when getting a new SVG icon may be not working (to be confirmed)

The big advantage is that you can now change the color of your icons (for colorless SVG icons).

This change will add nothing really new when using the classic iconset as all (or almost all ?) icons have hardcoded colors.

I could also make this new feature an option of Basic UI.

@lolodomo lolodomo marked this pull request as draft March 17, 2023 20:02
@lolodomo lolodomo changed the title [WIP][BasicUI] Use inline SVG for "colorless" SVG icons [BasicUI] Use inline SVG for "colorless" SVG icons Mar 17, 2023
@lolodomo lolodomo force-pushed the basicui_inline_svg branch from f9d4ab4 to e32544e Compare March 18, 2023 08:52
Related to openhab#1743

Use a HTML svg element (inline SVG) instead of a img element
when the icon servlet returns a SVG icon containing "currentColor".

Then it is possible to adjust the color of these icons by using the
iconcolor sitemapo attribute.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo force-pushed the basicui_inline_svg branch from e32544e to 5d3468b Compare March 18, 2023 12:25
@kaikreuzer
Copy link
Member

@lolodomo You asked for this to be reviewed, but the PR is still in DRAFT mode. Is this by accident?

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

This looks like some voodoo, but I trust you have tested it. 😄
Just one small comment below.

bundles/org.openhab.ui.basic/web-src/smarthome.js Outdated Show resolved Hide resolved
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 12, 2023

@lolodomo You asked for this to be reviewed, but the PR is still in DRAFT mode. Is this by accident?

My wish is to merge it before new milestone.
But I will add an option to let the user choose if he wants this feature or not. I will finish the PR probably Friday.

@lolodomo lolodomo marked this pull request as ready for review July 16, 2023 09:59
@lolodomo
Copy link
Contributor Author

lolodomo commented Jul 16, 2023

@kaikreuzer : PR is now fully ready for a merge.

With this sitemap:

	Frame label="Text icon color" {
		Default item=TestNumber icon="mic" iconcolor=[==56="blue", "yellow"]
		Default item=SetpointTemp icon="temperature" iconcolor=["yellow"]
	}

and the mic.svg being a custom icon containing currentColor as the fill color.

With the feature enabled: => inline SVG and color considered
image
image

With the feature disabled: => IMG and color ignored
image

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks!

@kaikreuzer kaikreuzer merged commit 9be4293 into openhab:main Jul 16, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone Jul 16, 2023
@lolodomo lolodomo deleted the basicui_inline_svg branch July 16, 2023 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants