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] Fix icon none #2231

Merged
merged 1 commit into from
Dec 22, 2023
Merged

[BasicUI] Fix icon none #2231

merged 1 commit into from
Dec 22, 2023

Conversation

mherwege
Copy link
Contributor

As described in https://community.openhab.org/t/openhab-4-1-milestone-discussion/149502/222, none.png is not found for BasicUI.

This PR replaces it with non.svg, see discussion on forum.

@lolodomo @florian-h05 I was behind my screen when I saw the issue. I didn't test, but did the replacement based on info provided on the forum.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from a team as a code owner December 22, 2023 09:41
Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I just wonder whether /images/none.png works, because the resource folder contains that image.
Anyway, I think this PR can be merged as a quick fix. I won‘t be at home in time so I cannot try out if there is a better solution.

@florian-h05
Copy link
Contributor

@kaikreuzer WDYT?

@kaikreuzer
Copy link
Member

I'm just looking at it - I would change as little as possible; as you say, the image exists, so it's rather a matter of fixing the icon. I'm trying to debug locally now.

@kaikreuzer
Copy link
Member

Argh, bad timing; my Internet connection just broken and I am also having trouble running the npm part of the Basic UI build locally -> I am failing to test it locally right now. 😢

@florian-h05
Copy link
Contributor

I‘m on the way home but it’ll definitely take till 1 pm … and unfortunately I also don’t have my notebook with me.

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.

In general, I would also say the fix looks good.
So let's merge it and quickly test it on a new snapshot.

@kaikreuzer kaikreuzer merged commit cdfe37a into openhab:main Dec 22, 2023
3 checks passed
@kaikreuzer kaikreuzer added bug Something isn't working regression labels Dec 22, 2023
@kaikreuzer kaikreuzer added this to the 4.1 milestone Dec 22, 2023
@florian-h05
Copy link
Contributor

And once the release is done we can have a look at fixing it in a cleaner way 👍

@kaikreuzer
Copy link
Member

I successfully tested the fix!

@lolodomo
Copy link
Contributor

Thank you guys.

Normally images/none.png should exist, it is one of the PNG file packaged with Basic UI. Here is the link:
https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/src/main/resources/web/images/none.png

So you probably changed more things than expected.

@mherwege
Copy link
Contributor Author

I probably have. I was mislead by https://community.openhab.org/t/openhab-4-1-milestone-discussion/149502/226 stating images/none.png not existing, but it was using the wrong path, as the URL would have to be http://openHAB:8080/basicui/images/none.png to test.

lolodomo pushed a commit that referenced this pull request Jan 2, 2024
See comment
#2231 (comment)

This is the minimal fix to the issue.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
lolodomo pushed a commit that referenced this pull request Jan 2, 2024
See comment
#2231 (comment)

This is the minimal fix to the issue.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@florian-h05 florian-h05 added the basic ui Basic UI label Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI bug Something isn't working regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants