♻️ Refactor: Move inline styles containing background-image: out of line#2268
Conversation
✅ Deploy Preview for snazzy-dango-efb2ec ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
5cdcb06 to
3446df0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
3446df0 to
20bbf3d
Compare
This greatly reduces the number of inline styles in the project and enhances maintainability by avoiding a lot of duplicate code. This also reduces the number of hashes required for a CSP since all previous background-image inline styles are now in main.css.
I'm not sure why this is such a big problem, but hugo doesn't seem to like this. It is also better to just use the intended way to reassign vars always. Before, *feature* images if using background.html would always get overwritten, now that is fixed.
e3e90cf to
8048636
Compare
|
@servedsmart I appreciate the work you've put into this PR, but I've noticed several performance concerns that might impact the build process:
I may have missed some implementation details, so please feel free to correct me if I'm wrong about any of these points. Since I'm not a collaborator or owner of this project, I won't be able to provide ongoing code review support. However, I'd suggest consulting with modern AI tools like ChatGPT or Claude - they're excellent at helping optimize Hugo templates and can provide detailed guidance on performance improvements. Thanks for understanding! |
Thank you for the feedback. I didn't think that build time was such a big concern, therefore I didn't bother. In absolute terms this should not increase build time by a lot, but I do understand and think that this is a valid point. I see that you have already taken care of some of your concerns in #2318. Thank you very much! Also for the fix, I really didn't consider that scenario. I will use functions to avoid code duplication in a new pr and address additional concerns of yours. Thank you for the again very valuable feedback! |
|
Thanks for the reply. I appreciate your response, though I did notice a few statements that seem based on assumptions rather than confirmed behavior. Phrases like “I didn't think...”, “should not increase...”, “I also thought...”, “quite minimal” come across as speculative, which can be risky when discussing performance and build behavior. To help move this forward constructively, I'd suggest checking the actual impact using profiling tools, or reaching out to the Hugo maintainers to confirm whether this pattern leads to measurable performance issues. It's important that these decisions are based on tested behavior rather than assumptions. Thanks again. |
I think there might be a misunderstanding about my CSS size concern. It's about your code generating dual resize CSS (1200x and 600x) for every image instead of distinguishing background from feature images. Since you mentioned CSS optimization, could you clarify how exactly this would work? Also, the current approach seems to make bundle.min.css unable to cache because adding images changes the CSS file. |
I know what you mean and I should have probably made it more clear.
I btw just wanted to point out why I did what I did, not say that my approach was not flawed in some ways.
|
I said the following:
by that I mean exactly what you have just stated: additional classes that are added to css even though they are not used in html. I hope you can now understand what I was trying to say. To avoid this caching issue, I could use a separate CSS file. Also how the resize can be handled is getting the background-images from the files that use them. I think you have already done that in #2318 Btw, as I have stated previously I am quite new to hugo/web development and it is not that easy to know how or even if you can do these things with hugo's go templates. Your changes and suggestions have helped me a lot and I hope that you can at least somewhat understand my point of view. |
|
@ZhenShuo2021 I will eventually resubmit this pr and try to address your concerns and also add profiles to the new pr. I am sorry that my testing didn't cover the config parameters not being set. |
|
The main concern is long-term maintainability and performance. While strict CSP can be valuable in some cases, for a static blog theme like this, the real-world benefit is quite limited, and the added complexity may cause more issues than it solves. No one can guarantee they’ll maintain their code indefinitely. If that happens, the cost of complexity becomes technical debt others will need to handle. I encourage you to use AI tools to help review your code or even ask LLM to point out issues. LLM has become very powerful recently. To be transparent, I am also using AI to translate this message because English is not my native language. You’re welcome to resubmit anytime. Just note that I’m not the repo owner, please tag @nunocoracao for final decisions. |
|
For context, the changes led to a 300% increase in build time on my M1 MacBook. |
This is not acceptable. Small issues like this add up and become real problems over time. |
This has been tested for around half an hour locally, also with
thumbAndBackgroundparam and otherwise default exampleSite config. I'm pretty sure that all scenarios where changed html files are loaded were checked.Purpose
This greatly reduces the number of inline styles in the project and enhances maintainability by avoiding a lot of duplicate code.
This also reduces the number of hashes required for a CSP since all previous background-image inline styles are now in main.css.