funkydan2 / alpha-church

Hugo theme for churches based on a html5up theme
https://alpha-church.netlify.app
Other
67 stars 58 forks source link

Banner background-image URLs are set to localhost:1313 rather than baseURL #28

Open gordonthegopher opened 4 years ago

gordonthegopher commented 4 years ago

When I test my site with "hugo server", the banner image comes up fine, but once I actually generate the site and upload the "public" folder, the banner doesn't work.

Looking at the Chrome console, it looks like for some reason the #banner CSS property is set to "url(http://localhost:1313/img/overlay.png),url(http://localhost:1313/img/banner.jpg)".

You can see this is the case by looking at the example site:

https://themes.gohugo.io/theme/alpha-church/

If you have a local Hugo server running, whatever you have as overlay.png and banner.jpg in your local public folder will show up on the example site! (as the images are being served from localhost:1313). As soon as you kill your local Hugo server, they go missing again.

It seems like, for some reason, the baseURL isn't being honoured when the site is generated, and the URL is somehow hard coded to http://localhost:1313 instead?

funkydan2 commented 4 years ago

Thanks @gordonthegopher.

I can see where the problem comes from

https://github.com/funkydan2/alpha-church/blob/9a7ba298b79b6a8859445189b38ff8429339d229/assets/scss/main.scss#L1377

Since it uses absURL, when the resources are generated when running hugo server it will resolve the url to url(http://localhost:1313/img/overlay.png),url(http://localhost:1313/img/banner.jpg).

Part of the problem is this theme ships with a version of the resources folder generated by running hugo server (that's recommended best practice)...and so localhost:1313 is hardcoded and shipped with the theme. I can fix this by changing to relURL.

However, I would have expected when you run hugo (as long as you're running the extended version of hugo) it would generate the url using baseURL.

Can you run hugo version and post the results here?

funkydan2 commented 4 years ago

I've just realised there is another problem for users who don't generate their own assets through the extended version of hugo - the css won't load banner image specified in config.toml'.

Maybe we should revert back to loading the background image through an inline style - I'll have to look into why that was changed.

gordonthegopher commented 4 years ago

Thanks for getting back to me!

However, I would have expected when you run hugo (as long as you're running the extended version of hugo) it would generate the url using baseURL.

Can you run hugo version and post the results here?

Hugo Static Site Generator v0.63.2-934EE21F linux/amd64 BuildDate: 2020-01-27T12:13:19Z

I installed this manually from the .deb on Ubuntu 18.04 (the version from the repos is really old - 0.40.1 and the Snap package doesn't play nicely with here I keep my files as it's outside /home)

I've just realised there is another problem for users who don't generate their own assets through the extended version of hugo - the css won't load banner image specified in config.toml'.

Yup, I noticed this also, whatever I changed it to, it would still always reference "/img/banner.jpg"

Maybe we should revert back to loading the background image through an inline style - I'll have to look into why that was changed.

I just issued "git submodule update --rebase --remote" in the root of my site folder, but the site generated still has the same issue.

So, I installed the Extended version of 0.63.2 from the .deb manually and the banner now loads as expected.

Should this be marked as closed now?

It seems like my issue is related to not using the Extended version of Hugo specifically, right?

funkydan2 commented 4 years ago

Thanks @gordonthegopher. The issue is a mixture of (1) using absURL rather than relURL in the SCSS file and (2) not having access to the extended version of Hugo. But (2) isn't your fault - best practice for Hugo is to build themes that work regardless of whether the extended version is available.

I've just now pushed a commit - so if you rebase the theme now, you should get a version that you can make to work. And yes, you'll have to put your banner image at /img/banner.jpg, as the parameter in the config file won't make any difference without Hugo extended.

As you can see just above your latest post, I've asked a question for the person who contributed the PR with the code which caused you problems. I might get an idea of a better fix from there.

Anaeijon commented 4 years ago

came here from #15

Last time I touched a project based on your theme, was nearly a year ago... I'm looking into it right now. The project needs migration and a few new images anyway, and when I'm already working on it, I might as well look into this.

The main point of #15 was, that the inline-CSS attribute background-image always enforced the use of overlay.png from within the theme folder. So if I didn't want the overlay, I would have needed to find the CSS attribute in the theme folder and change it manually, creating a fork of the theme. If I would want to change the overlay, I would need to edit the PNG in the theme-folder. Also, there was no option to simply not show a background image at all and use some CSS gradient or color instead. The option of overloading CSS attributes in the custom.css gets defeated, once you introduce inline-Styles in the HTML.

Also there was no option to include additional google-fonts and users were basically limited to Source Sans Pro (I think). I wanted Playfair Display and Roboto, so I thought, it would make sense giving the options in the config. There is no other way of achieving that, without simply making the options apply in the SCSS.

The problem that I see, is, that either you completely avoid inline-styles in HTML and therefore allow overloading via custom CSS or you use inline-styles in HTML but in that case you need to add all possible variations on these inline-styles in form of options in the toml-File, to allow users to change them.

I'm not really fluent in the Hugo-syntax anymore and I'm not sure what you mean by "extended version". I'll look into it later again.

I'm personally not using the controversial changes from #15 anymore, except the Font-Import. As long as there is an option to import fonts, I wouldn't mind reverting it. The additional options for contact.html (which I also included in #15 for whatever reason) aren't affected by this and should have been requested separately by me.

Edit: I just saw, that this is slightly related to #16, the other issue we discussed last year. I would propose, officially switching to Hugo Extended since it allows to keep the codebase clean and doesn't require commits to include various automatically generated files in _gen. I guess, the second would also be able to be done with the non-extended version by now. I was trying to find out what you did in the most recent commit and had to scroll through 27 automatically generated files, just to find the 1 line that actually contains the change. The reason for this, netlify using outdated hugo version, is fixed for a while now. The only thing needed to execute this with hugo without hugo extended would be a precompiled CSS file. Since this project uses SCSS a lot, there basically is no way of effectively customising it without hugo extended. Sure, one could run the example site without hugo extended, but why would anybody want to do that? You are already hosting the example, have preview images included and can add hugo-extended to CI-CD.

Or am I missing something here?

funkydan2 commented 4 years ago

Thanks @Anaeijon, that's a very thoughtful response.

To answer the simplest question: the Hugo binary is released in two versions, an extended and a non-extended version. I'm not sure why. But because of this, the Hugo themes team requires the resources to be distributed with the theme.

I'm thinking that even though the current solution (somewhat) locks users of the non-extended version of Hugo to a particular filename for the banner, it's a sensible default. And even users of the non-extended version can work around this by including some customised CSS.

The change to relURL is a good solution for now, as it prevents the problem @gordonthegopher came across, where localhost:1313 was hardcoded into the packaged CSS file.

Finally, even if we were to change how the background image is loaded, there's no need to remove the method of customising fonts.