feat: performance optimizations #551
Conversation
|
Hi there! 👋 Thank you for your contribution to the FIP Guide! 🚀 Checklist before merging:
|
therobrob
added
the
technical
✅ Deploy Preview for fipguide ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Unfortunately, there is an error: Uncaught ReferenceError: PagefindUI is not defined. |
MoritzWeber0
left a comment
MoritzWeber0
left a comment
There was a problem hiding this comment.
Thanks for the effort to merge the pagefind styles 🚀
It's great if we no longer have to load thepagefind-ui CSS file :)
Unfortunately, there are a few styling issues when I run it locally, not sure if it's just my local preview that has those issues. I added comments for each issue that I found, sorry for the amount 😅
| <div class="o-startpage__image"> | ||
| {{ $image := resources.Get "images/startpage.webp" }} | ||
| {{ partial "image" (dict "image" $image "eager" true) }} | ||
| {{ partial "image" (dict "image" $image "eager" true "fetchpriority" "high") }} |
There was a problem hiding this comment.
Why do we want to have a high fetch priority for the start page image? In my opinion it's only decorative and should be loaded after other external resources like fonts, scripts and the operator logos or flags. So I'd set it to low or auto instead of high.
| ascent-override: 93%; | ||
| descent-override: 24%; | ||
| line-gap-override: 0%; |
There was a problem hiding this comment.
These adjustments help match the fallback font, right? That makes sense.
I noticed the text on "National Rail" looks slightly different in terms of centering, but it's quite subtle and probably won't be noticeable to others.
Before (left) vs After (right)
assets/sass/search.scss
Outdated
| // --------------------------------------------- | ||
| // SEARCH INPUT | ||
| // --------------------------------------------- |
There was a problem hiding this comment.
Good comments should not repeat the code. Since the class is named search-input, the comment doesn't provide any additional information. I assume it was added by Copilot (and Copilot loves adding comments), so I've removed it and the other comments.
Feel free to state your opinion if it doesn't match mine, then I can also add them back.
Here is a good read about best practices when writing comments: https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/#h2-b7593c698a940
There was a problem hiding this comment.
Black text on dark background in dark mode (in prod it works)
There was a problem hiding this comment.
The first row (8 Ergebnisse für Eurostar) is not well aligned anymore. In prod it's left-aligned:
There was a problem hiding this comment.
The arrow right icon seems to be broken (Firefox only):
There was a problem hiding this comment.
On production, the title has a larger font size than the other text:
We should add it back.
There was a problem hiding this comment.
The margins around "Mehr Ergebnisse laden" are not equal, in production they are. Would
There was a problem hiding this comment.
The font of the search seems to be Arial, no longer Roboto in Chrome:
There was a problem hiding this comment.
In the deployment preview, the search doesn't load anymore:
https://deploy-preview-551--fipguide.netlify.app/de/
MoritzWeber0
dismissed
their stale review
Wanted to approve the other PR, I should get some sleep xD
I’ve experimented with some performance optimizations. Feel free to have a critical view. :)
fetchpriority=high