Add village map (needs more work) - #750
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces an ASCII-art village map feature to display a visual representation of the village in the "Outside" module. The PR also includes comprehensive code formatting improvements across HTML files and adds a package-lock.json file for dependency management. While the feature represents an interesting UI enhancement, there are several logic issues in the implementation that need to be addressed.
Key Changes:
- Added a static
village_mapASCII-art string to the Outside module that displays a visual representation of the village with huts (⌂), terrain (., ~, ^), and trees (,) - Modified
updateVillageRowto handle rendering the village map as a special row type, though with some implementation issues - Added CSS styling with monospace font for proper ASCII-art display
- Reformatted index.html, mobileWarning.html with consistent 4-space indentation and improved readability
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| script/outside.js | Added village_map ASCII art and modified updateVillageRow to display it, with logic issues in conditional flow and code duplication |
| css/outside.css | Added .village_map CSS class with monospace font styling for proper ASCII art rendering |
| index.html | Reformatted with consistent indentation and improved spacing throughout HTML and JavaScript sections |
| mobileWarning.html | Reformatted with consistent indentation and improved spacing for better readability |
| package-lock.json | Added complete dependency lock file for Express 4.17.1 and its dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $('div#' + row.attr('id') + ' > div.row_val', village).text(Outside.village_map); | ||
| return; | ||
| } | ||
| if (row.length <= 0) row.remove(); |
There was a problem hiding this comment.
The logic in updateVillageRow for handling the 'village' map has flawed conditional structure. The condition else if (name === 'village') on line 436 will never be reached when num is null (as called on line 489), because the previous conditions check num > 0 and num === 0, leaving only null, undefined, or negative numbers to fall through. However, null is not strictly equal to 0 or greater than 0, so it should fall through correctly.
The real issue is on line 441: if (row.length <= 0) row.remove(); - this attempts to call remove() on a jQuery result that is already empty (length 0), and more critically, it reassigns row immediately after on line 442, making this line redundant and confusing. The check should be removed entirely as it serves no purpose.
| if (row.length <= 0) row.remove(); |
| updateVillageRow: function (name, num, village) { | ||
| var id = 'building_row_' + name.replace(' ', '-'); | ||
| var lname = _(name); | ||
| var row = $('div#' + id, village); | ||
| if (row.length === 0 && num > 0) { | ||
| row = $('<div>').attr('id', id).addClass('storeRow'); | ||
| $('<div>').addClass('row_key').text(lname).appendTo(row); | ||
| $('<div>').addClass('row_val').text(num).appendTo(row); | ||
| $('<div>').addClass('clear').appendTo(row); | ||
| var curPrev = null; | ||
| village.children().each(function (i) { | ||
| var child = $(this); | ||
| if (child.attr('id') != 'population') { | ||
| var cName = child.children('.row_key').text(); | ||
| if (cName < lname) { | ||
| curPrev = child.attr('id'); | ||
| } | ||
| } | ||
| }); | ||
| if (curPrev == null) { | ||
| row.prependTo(village); | ||
| } else { | ||
| row.insertAfter('#' + curPrev); | ||
| } | ||
| } else if (num > 0) { | ||
| $('div#' + row.attr('id') + ' > div.row_val', village).text(num); | ||
| } else if (num === 0) { | ||
| row.remove(); | ||
| } else if (name === 'village') { | ||
| if (row.length > 0) { | ||
| $('div#' + row.attr('id') + ' > div.row_val', village).text(Outside.village_map); | ||
| return; | ||
| } | ||
| if (row.length <= 0) row.remove(); | ||
| row = $('<div>').attr('id', id).addClass('storeRow'); | ||
| $('<div>').addClass('row_key').text(lname).appendTo(row); | ||
| $('<div>').addClass('row_val village_map').text(Outside.village_map).appendTo(row); | ||
| $('<div>').addClass('clear').appendTo(row); | ||
| var curPrev = null; | ||
| village.children().each(function (i) { | ||
| var child = $(this); | ||
| if (child.attr('id') != 'population') { | ||
| var cName = child.children('.row_key').text(); | ||
| if (cName < lname) { | ||
| curPrev = child.attr('id'); | ||
| } | ||
| } | ||
| }); | ||
| if (curPrev == null) { | ||
| row.prependTo(village); | ||
| } else { | ||
| row.insertAfter('#' + curPrev); | ||
| } |
There was a problem hiding this comment.
The 'village' map handling logic in updateVillageRow contains significant code duplication (lines 442-460) that replicates the row creation and insertion logic from lines 413-431. This makes the function harder to maintain and more error-prone. Consider refactoring to extract the common row creation and positioning logic into a separate helper function, or restructure the conditions to handle the 'village' case within the existing flow by treating it as a special type of row.
| updateVillageRow: function (name, num, village) { | |
| var id = 'building_row_' + name.replace(' ', '-'); | |
| var lname = _(name); | |
| var row = $('div#' + id, village); | |
| if (row.length === 0 && num > 0) { | |
| row = $('<div>').attr('id', id).addClass('storeRow'); | |
| $('<div>').addClass('row_key').text(lname).appendTo(row); | |
| $('<div>').addClass('row_val').text(num).appendTo(row); | |
| $('<div>').addClass('clear').appendTo(row); | |
| var curPrev = null; | |
| village.children().each(function (i) { | |
| var child = $(this); | |
| if (child.attr('id') != 'population') { | |
| var cName = child.children('.row_key').text(); | |
| if (cName < lname) { | |
| curPrev = child.attr('id'); | |
| } | |
| } | |
| }); | |
| if (curPrev == null) { | |
| row.prependTo(village); | |
| } else { | |
| row.insertAfter('#' + curPrev); | |
| } | |
| } else if (num > 0) { | |
| $('div#' + row.attr('id') + ' > div.row_val', village).text(num); | |
| } else if (num === 0) { | |
| row.remove(); | |
| } else if (name === 'village') { | |
| if (row.length > 0) { | |
| $('div#' + row.attr('id') + ' > div.row_val', village).text(Outside.village_map); | |
| return; | |
| } | |
| if (row.length <= 0) row.remove(); | |
| row = $('<div>').attr('id', id).addClass('storeRow'); | |
| $('<div>').addClass('row_key').text(lname).appendTo(row); | |
| $('<div>').addClass('row_val village_map').text(Outside.village_map).appendTo(row); | |
| $('<div>').addClass('clear').appendTo(row); | |
| var curPrev = null; | |
| village.children().each(function (i) { | |
| var child = $(this); | |
| if (child.attr('id') != 'population') { | |
| var cName = child.children('.row_key').text(); | |
| if (cName < lname) { | |
| curPrev = child.attr('id'); | |
| } | |
| } | |
| }); | |
| if (curPrev == null) { | |
| row.prependTo(village); | |
| } else { | |
| row.insertAfter('#' + curPrev); | |
| } | |
| /** | |
| * Helper to create and insert a village row. | |
| * @param {string} id - The row id. | |
| * @param {string} keyText - The key text. | |
| * @param {string|number} valueText - The value text. | |
| * @param {string} valueClass - The class(es) for the value div. | |
| * @param {object} village - The parent container. | |
| */ | |
| createAndInsertVillageRow: function(id, keyText, valueText, valueClass, village) { | |
| var row = $('<div>').attr('id', id).addClass('storeRow'); | |
| $('<div>').addClass('row_key').text(keyText).appendTo(row); | |
| $('<div>').addClass(valueClass).text(valueText).appendTo(row); | |
| $('<div>').addClass('clear').appendTo(row); | |
| var curPrev = null; | |
| village.children().each(function (i) { | |
| var child = $(this); | |
| if (child.attr('id') != 'population') { | |
| var cName = child.children('.row_key').text(); | |
| if (cName < keyText) { | |
| curPrev = child.attr('id'); | |
| } | |
| } | |
| }); | |
| if (curPrev == null) { | |
| row.prependTo(village); | |
| } else { | |
| row.insertAfter('#' + curPrev); | |
| } | |
| return row; | |
| }, | |
| updateVillageRow: function (name, num, village) { | |
| var id = 'building_row_' + name.replace(' ', '-'); | |
| var lname = _(name); | |
| var row = $('div#' + id, village); | |
| if (name === 'village') { | |
| if (row.length > 0) { | |
| $('div#' + row.attr('id') + ' > div.row_val', village).text(Outside.village_map); | |
| return; | |
| } | |
| if (row.length <= 0) row.remove(); | |
| Outside.createAndInsertVillageRow(id, lname, Outside.village_map, 'row_val village_map', village); | |
| } else if (row.length === 0 && num > 0) { | |
| Outside.createAndInsertVillageRow(id, lname, num, 'row_val', village); | |
| } else if (num > 0) { | |
| $('div#' + row.attr('id') + ' > div.row_val', village).text(num); | |
| } else if (num === 0) { | |
| row.remove(); |
| row.remove(); | ||
| } else if (name === 'village') { | ||
| if (row.length > 0) { | ||
| $('div#' + row.attr('id') + ' > div.row_val', village).text(Outside.village_map); |
There was a problem hiding this comment.
The control flow in the 'village' map case is problematic. When row.length > 0 (line 437), the function updates the text and returns early (line 439). However, if row.length is 0, the code proceeds to line 441 which has a redundant check and then creates a new row. This means that the village_map class will only be applied when creating a new row (line 444), but not when updating an existing row (line 438). This inconsistency could lead to the monospace font styling not being applied if the row already exists when it's updated.
| $('div#' + row.attr('id') + ' > div.row_val', village).text(Outside.village_map); | |
| $('div#' + row.attr('id') + ' > div.row_val', village).addClass('village_map').text(Outside.village_map); |
| village_map: ` | ||
| ,,,,,,,,,,,,,,,,,,,,, | ||
| ,,,,,,,⌂,,,,,,,,,,,,, | ||
| ,,,,,,,,,,,,,⌂,,,,⌂,, | ||
| ,,⌂,,,⌂,,...,,,,,,,,, | ||
| ,,,,⌂,,...~...,⌂,,,,, | ||
| ,,,,,,,..~^~..,,,,,,, | ||
| ,,,,⌂,,...~...,,,⌂,,, | ||
| ,,,,,,,,,...,⌂,,,,,,, | ||
| ,,⌂,,,,⌂,,,,,,,,,⌂,,, | ||
| ,,,,,,,,,⌂,,,,⌂,,,,,, | ||
| ,,,,,,,,,,,,,,,,,,,,, | ||
| `, |
There was a problem hiding this comment.
The village_map property is defined as a module-level constant within the Outside object, but it represents static game content that should ideally be stored in the game state (as noted in the PR description) to persist across saves/loads. Consider documenting this as a TODO or storing it in a more appropriate location for game data that may need to be saved.
| if (!lang) { | ||
| try { | ||
| lang = localStorage.lang; | ||
| } catch (e) { |
There was a problem hiding this comment.
The empty catch block silently swallows exceptions when accessing localStorage.lang. While this may be intentional to handle browsers with localStorage disabled, it's better practice to either add a comment explaining why the exception is being ignored, or handle it minimally (e.g., catch (e) { /* localStorage not available */ }) to make the intent clear to future maintainers.
| } catch (e) { | |
| } catch (e) { | |
| // localStorage not available or disabled; ignore and proceed |
| }, | ||
|
|
||
| makeWorkerRow: function (key, num) { | ||
| name = Outside._INCOME[key].name; |
There was a problem hiding this comment.
Variable name is used like a local variable, but is missing a declaration.
| name = Outside._INCOME[key].name; | |
| var name = Outside._INCOME[key].name; |
| <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.10.1/jquery.min.js"></script> | ||
| <script> | ||
| if (!window.jQuery) { | ||
| document.write('<script src="lib/jquery.min.js"><\/script>') |
There was a problem hiding this comment.
Avoid using functions that evaluate strings as code.
| document.write('<script src="lib/jquery.min.js"><\/script>') | |
| var script = document.createElement('script'); | |
| script.src = 'lib/jquery.min.js'; | |
| document.head.appendChild(script); |
| document.write('<script src="lang/' + lang + '/strings.js"><\/script>'); | ||
| document.write('<link rel="stylesheet" type="text/css" href="lang/' + lang + '/main.css" \/>'); |
There was a problem hiding this comment.
Avoid using functions that evaluate strings as code.
| document.write('<script src="lang/' + lang + '/strings.js"><\/script>'); | |
| document.write('<link rel="stylesheet" type="text/css" href="lang/' + lang + '/main.css" \/>'); | |
| // Validate lang to allow only safe language codes (alphanumeric, hyphens, underscores) | |
| if (/^[a-zA-Z0-9_-]+$/.test(lang)) { | |
| var script = document.createElement('script'); | |
| script.src = 'lang/' + lang + '/strings.js'; | |
| document.head.appendChild(script); | |
| var link = document.createElement('link'); | |
| link.rel = 'stylesheet'; | |
| link.type = 'text/css'; | |
| link.href = 'lang/' + lang + '/main.css'; | |
| document.head.appendChild(link); | |
| } |
| // if a language different than english requested, load all translations | ||
| if (lang && lang != 'en') { | ||
| document.write('<script src="lang/' + lang + '/strings.js"><\/script>'); | ||
| document.write('<link rel="stylesheet" type="text/css" href="lang/' + lang + '/main.css" \/>'); |
There was a problem hiding this comment.
Avoid using functions that evaluate strings as code.
| document.write('<link rel="stylesheet" type="text/css" href="lang/' + lang + '/main.css" \/>'); | |
| var link = document.createElement('link'); | |
| link.rel = 'stylesheet'; | |
| link.type = 'text/css'; | |
| link.href = 'lang/' + lang + '/main.css'; | |
| document.head.appendChild(link); |
| <path d="m 18.024533,28.5722 c 2.532365,-2.243 5.064679,-4.4861 7.596993,-6.7292 4.907813,0 9.815625,0 14.723438,0 0,-6.8136 0,-13.6272 0,-20.4408 -12.976656,0 -25.953312,0 -38.9299676,0 0,6.8136 0,13.6272 0,20.4408 3.2917905,0 6.5835811,0 9.8753716,0 -0.643311,2.2431 -1.286622,4.4861 -1.9299604,6.7292 2.5323644,-2.243 5.0646784,-4.4861 7.5969924,-6.7292 0.999066,0 1.998131,0 2.997197,0 -0.643345,2.2431 -1.286691,4.4861 -1.930064,6.7292 z" style="stroke-width:1.0;fill:none;stroke-linejoin:miter;stroke-miterlimit:10;stroke-dasharray:none"></path> | ||
| <div id="wrapper"> | ||
| <div id="saveNotify"> | ||
| <script>document.write(_("saved."));</script> |
There was a problem hiding this comment.
Avoid using functions that evaluate strings as code.
|
i only code z80 and (real) zx spectrum basic about this village map, do workplaces belong to it? |
This pull request introduces a new ASCII-art style village map feature to the UI, improves code formatting and readability in
index.htmlandmobileWarning.html, and updates the logic for displaying the village map within the village section. The most significant changes are grouped below by theme.Village Map Feature:
village_mapASCII-art string to theOutsideobject inscript/outside.js, representing the village visually.updateVillageRowmethod to render the village map when the row name is'village', ensuring it is inserted in the correct order within the population display..village_mapincss/outside.cssfor proper ASCII-art display.Code Formatting and Readability:
index.htmlandmobileWarning.htmlto enhance readability and maintain consistency, including better formatting for the Google Tag Manager script and SVG logo. [1] [2] [3] [4] [5]Notes: