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 issue incorrect paths for fonts. Add methods to Use relative paths #179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nikrou
Copy link

@nikrou nikrou commented Oct 11, 2017

The idea is to allow to use relative path for fonts and/or images.

Copy link
Collaborator

@Lyrkan Lyrkan left a comment

Choose a reason for hiding this comment

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

Some comments I have after a quick look at your changes (one of them probably being related to the failing tests).

I would also add a test case for each method in test\index.js to be sure that the return this doesn't silently disappear later on.

As I said in #85 though, I'm not sure that's the right way to handle it. The file-loader seems to provide a useRelativePath option that could do the job (didn't have the time to test it, so I may be wrong).

@@ -0,0 +1,9 @@
$font-path: '../fonts';
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should probably be in the fixture/css folder.

@@ -75,6 +75,18 @@ const publicApi = {
return this;
},

setFontsPublicPath(publicPath) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add at least a small comment here with some use cases.

return this;
},

setImagesPublicPath(publicPath) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing comment for that one too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

By the way, do we really need two methods? is there a use case in which we would only call one of them or call them with different values?

@weaverryan
Copy link
Member

Yea, we need to do some playing with this. I'm also interested in useRelativePath, but it may have some issues: webpack-contrib/file-loader#150... though I think just using the relative paths for publicPath also has issues... @Lyrkan we need to do some experimenting with this. I think it's unlikely that we'll be able to change Encore to have useRelativePath by default (again, because I think there are issues), but we might make a way to opt into this. I think we need experimenting to know more, then we can find the best solution.

@scaytrase
Copy link

@weaverryan any news on relative paths?

@LeoShivas
Copy link

Hello,

I also need relative paths for fonts. Any news ?

@weaverryan weaverryan changed the base branch from master to main November 18, 2020 18:22
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.

None yet

5 participants