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

fix: replace backslash with slash so to support rendering on Windows (#3120) #3122

Merged
merged 3 commits into from Jul 6, 2023

Conversation

lstocchi
Copy link
Contributor

@lstocchi lstocchi commented Jul 5, 2023

What does this PR do?

This PR replaces backslashes with slashes when creating the url of a font style.
On Windows backslashes get removed and it results in a wrong url.

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

this fixes #3120

How to test this PR?

N/A
This is useful for #1899 to work

@lstocchi lstocchi requested review from a team and benoitf as code owners July 5, 2023 13:51
@lstocchi lstocchi requested review from jeffmaury and cdrage and removed request for a team July 5, 2023 13:52
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

works for me but as you suggested, maybe it's better to have two fields instead of one field

@@ -46,7 +46,7 @@ onMount(() => {
});

fontsToAdd.forEach(font => {
const src = font.src.map(l => `${toUrl(l.location)} format('${l.format}')`).join(', ');
const src = font.src.map(l => `${toUrl(l.browserURL)} format('${l.format}')`).join(', ');
Copy link
Collaborator

@benoitf benoitf Jul 5, 2023

Choose a reason for hiding this comment

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

maybe it means we wouldn't need the toUrl method ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought there was a reason to have url('file://${location.replace(/'/g, '%27')}') in the client. Do i move everything in the server?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that now that we provide the field for the client on the server, we could do everything on the server side so client is dummy, just use the field

Copy link
Contributor

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

Worked for me LGTM!

Signed-off-by: lstocchi <lstocchi@redhat.com>
@lstocchi
Copy link
Contributor Author

lstocchi commented Jul 5, 2023

@benoitf updated. Please give it another look

@lstocchi lstocchi requested a review from benoitf July 5, 2023 21:18
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

LGTM but we have a style error:

Strings must use singlequote

Signed-off-by: lstocchi <lstocchi@redhat.com>
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

approving again

@benoitf
Copy link
Collaborator

benoitf commented Jul 6, 2023

thanks @lstocchi

@lstocchi lstocchi merged commit 314a8a6 into containers:main Jul 6, 2023
8 checks passed
@lstocchi lstocchi deleted the i3120 branch July 6, 2023 07:29
@podman-desktop-bot podman-desktop-bot added this to the 1.2.0 milestone Jul 6, 2023
mairin pushed a commit to mairin/podman-desktop that referenced this pull request Jul 7, 2023
…ontainers#3120) (containers#3122)

* fix: replace backslash with slash so to support rendering on Windows (containers#3120)

Signed-off-by: lstocchi <lstocchi@redhat.com>

* generate whole browser url from server side

Signed-off-by: lstocchi <lstocchi@redhat.com>

* fix: lint warning

Signed-off-by: lstocchi <lstocchi@redhat.com>

---------

Signed-off-by: lstocchi <lstocchi@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Font url not escaped correctly when retrieving the resource in Windows
4 participants