fix(ui): prevent mobile header from cutting off install button - #8
Conversation
…x css cache busting
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses two key areas: improving the user interface on mobile devices and enhancing backend cache management. The UI changes ensure a better experience for mobile users by preventing header elements from overlapping or being truncated. Concurrently, the cache busting mechanism has been made more robust to ensure all CSS assets are properly updated, preventing stale styles from being served. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully addresses the UI issue on mobile where the header title could overflow and cut off the install button. The CSS changes are well-implemented to handle this. The fix for cache-busting CSS files is also good, but it introduces an opportunity to improve the cache-busting for JavaScript files and fixes a related bug where one JS file was missed. I've left a comment with a suggestion for this improvement.
|
|
||
| // Dynamically cache-bust main static assets based on package version | ||
| text = text.replace(/href="static\/css\/style\.css"/g, `href="static/css/style.css?v=${version}"`); | ||
| text = text.replace(/href="static\/css\/([^"]+)\.css"/g, `href="static/css/$1.css?v=${version}"`); |
There was a problem hiding this comment.
This is a great improvement for cache-busting CSS files. You could apply a similar pattern for the JavaScript files below. This would make the code more maintainable and also fix a bug where location.js is not being cache-busted.
I recommend replacing all the individual text.replace calls for JS files (lines 173-177) with a single, more robust line:
text = text.replace(/src="static\/js\/([^\"]+)\.js"/g, `src="static/js/$1.js?v=${version}"`);
Fixes the header title overflowing and cutting off the install button on mobile devices. Also fixes a cache busting bug in
handlers.mjswhere onlystyle.csswas targeted instead of all css files.