Conversation
mjumbewu
left a comment
There was a problem hiding this comment.
Hey Ryan, this is a visually impressive submission. A few things I really liked:
- The visual design is clean -- the map colors, icons, and tiles tie in with the other UI components, and they're all styled to subtly reference the style of the car itself; well done
- The correlation between the map data and the story is tight
- The narrative is written as if you're actually interested in the topic, which I like to see
Some areas for improvement:
-
JavaScript Version: I noticed you have a .backup.js file in your folder that uses modern JavaScript syntax (like
const,let, andasync/await), but your main porsche-911-story.js file uses older syntax (likevar). The code in your backup file is actually the standard I prefer! Did you run into a specific issue that made you switch to the older syntax? Is that an AI recommendation (no shade)? I'd encourage you to use the modern approach from your backup file. -
Linting Errors: There are quite a few linting errors in your JavaScript and CSS code. Many of these could be fixed automatically by running
npx eslint --fix .andnpx stylelint --fix "./**/*.css"in your project folder. This will help ensure your code follows best practices and is easier to read. However, because of all thevarusage, as mentioned above, there will be a number of JS linting errors that the linter can't fix automatically; you'll need to address those manually. -
Map Padding: On desktop, your story panel covers the right side of the map. When the map zooms to some data, part of it often gets hidden behind that panel. You can fix this by adding
paddingBottomRight: [420, 0](or however wide your panel is) to yourmap.flyToBoundsoptions. This tells Leaflet to 'center' the map in the remaining space. You could even match the padding to your media queries, e.g.:const options = window.matchMedia("(width >= 960px)").matches ? { paddingBottomRight: [420, 0] } : {}; map.flyToBounds(bounds, options);
-
Alt Text: Check the alt text on your markers. Since you have such rich data, try to make the alt text descriptive of the specific location rather than just a generic label.
-
Project Structure: Currently, your code is buried deep in templates/storypages/.... You should rename your main HTML file to index.html, and move it along with your css, js, and data folders to the root of your repository. Then you should enable a GitHub Pages action (e.g. Settings > Pages > Build and deployment and choose "GitHub Actions" as the source) so that the project is viewable online.
| font-size: 0.95rem; | ||
| } | ||
|
|
||
| @media (max-width: 1200px) { |
There was a problem hiding this comment.
suggestion: Though AI often recommends using the max-width syntax, I recommend switching to the inequality operator syntax, as they're more explicit and flexible. I.e.
| @media (max-width: 1200px) { | |
| @media (width <= 1200px) { |
AI will catch on one day.
There was a problem hiding this comment.
suggestion: I recommend renaming this file to index.html, as that will make it the "default" file in the folder.
| @@ -0,0 +1,721 @@ | |||
| (function() { | |||
There was a problem hiding this comment.
This syntax is called an Immediately Invoked Function Expressions (IIFEs) and used to be used before JS modules were a thing. These days they're entirely unnecessary when writing JS, if you use a <script type="module" src="..."> in your HTML.
| var disclaimerBackdrop = document.getElementById("disclaimer-backdrop"); | ||
| var disclaimerPanel = document.getElementById("disclaimer-panel"); | ||
| var disclaimerClose = document.getElementById("disclaimer-close"); | ||
| var disclaimerContinue = document.getElementById("disclaimer-continue"); | ||
| var disclaimerDismissed = false; |
There was a problem hiding this comment.
suggestion: Using const and let achieves two things:
- Protects me from overwriting values that I don't intend to later on, and
- Communicates to someone reading your code later on which variables are intended to change and which are not.
| var disclaimerBackdrop = document.getElementById("disclaimer-backdrop"); | |
| var disclaimerPanel = document.getElementById("disclaimer-panel"); | |
| var disclaimerClose = document.getElementById("disclaimer-close"); | |
| var disclaimerContinue = document.getElementById("disclaimer-continue"); | |
| var disclaimerDismissed = false; | |
| const disclaimerBackdrop = document.getElementById("disclaimer-backdrop"); | |
| const disclaimerPanel = document.getElementById("disclaimer-panel"); | |
| const disclaimerClose = document.getElementById("disclaimer-close"); | |
| const disclaimerContinue = document.getElementById("disclaimer-continue"); | |
| let disclaimerDismissed = false; |
| function loadJson(url) { | ||
| return fetch(url).then(function(resp) { | ||
| if (!resp.ok) throw new Error('Failed to load ' + url); | ||
| return resp.json(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Here's the exact same function with async/await. Which style you choose is mostly a matter of aesthetic preference.
| function loadJson(url) { | |
| return fetch(url).then(function(resp) { | |
| if (!resp.ok) throw new Error('Failed to load ' + url); | |
| return resp.json(); | |
| }); | |
| } | |
| async function loadJson(url) { | |
| const resp = await fetch(url); | |
| if (!resp.ok) throw new Error('Failed to load ' + url); | |
| return resp.json(); | |
| } |
This is a project by Yu Qiushi illustrating the production and delivery process of a Porsche 911 car from Germany to the US. It is still in active work. Please note that I used a lot of vibe coding to save my time and GPT to refine my writing, but the main idea is from myself, and I planned the project structure.