Feedback #1

github-classroom · 2024-03-09T12:59:15Z

👋! GitHub Classroom created this pull request as a place for your teacher to leave feedback on your work. It will update automatically. Don’t close or merge this pull request, unless you’re instructed to do so by your teacher.
In this pull request, your teacher can leave comments and feedback on your code. Click the Subscribe button to be notified if that happens.
Click the Files changed or Commits tab to see all of the changes pushed to main since the assignment started. Your teacher can see this too.

Notes for teachers

Use this PR to leave feedback. Here are some tips:

Click the Files changed tab to see all of the changes pushed to main since the assignment started. To leave comments on specific lines of code, put your cursor over a line of code and click the blue + (plus sign). To learn more about comments, read “Commenting on a pull request”.
Click the Commits tab to see the commits pushed to main. Click a commit to see specific changes.
If you turned on autograding, then click the Checks tab to see the results.
This page is an overview. It shows commits, line comments, and general comments. You can leave a general comment below.
For more information about this pull request, read “Leaving assignment feedback in GitHub”.

Subscribed: @avauga03

joemull

Really excellent work! I have left some detailed comments here on GitHub, but a summary and mark will also be sent out later.

joemull · 2024-05-25T10:06:35Z

script.js

+    document.querySelectorAll(`[data-country="${path.dataset.country}"]`).forEach(svgPath => {
+        svgPath.classList.toggle('hover-effect', highlight);
+    });
+}


This is great--I can see that you are solving a problem where the path being hovered over may just be one of several areas that make up that country.

joemull · 2024-05-25T10:28:51Z

script.js

+function countryHighlights(genre, bookData) {
+    document.querySelectorAll('.genre-highlight').forEach(path => path.classList.remove('genre-highlight'));
+    Object.values(bookData).forEach(geographicalArea  => {
+        (geographicalArea[genre] || []).forEach(book => {


I understand the desire to make things as compact as possible, and it's a common style that you see in JavaScript out in the world, including in guides and demos. But I would also encourage you to balance it with things that help with readability, like assigning things to descriptive variable names and avoiding executing methods on anonymous expressions.

Line 85 is an example of a method being executed on an anonymous expression, and it is a bit hard to read. I'd maybe add a variable name with something like this:

const booksInGenre = geographicalArea[genre] || [] booksInGenre.forEach( etc. )

This will help people who aren't as familiar with the data structure in booksData.json, and it will also help you later when you've forgotten the details and are coming back to your own code.

joemull · 2024-05-25T10:38:13Z

script.js

+    } else {
+        bookInfo.innerHTML += `<p>There are no books found for ${genre} in ${country}.</p>`;
+    }
+}


This is quite a well designed function. The variables that you set at each level before going deeper in the data structure help a lot with readability.

Also, great use of filter.

joemull · 2024-05-25T10:39:02Z

script.js

+            countryHighlights(genreSelect.value, data); 
+        })
+        .catch(error => console.error('There is an error when trying to fetch the book data:', error));
+});


Great program design--this is the "controller" for your application's logic, and it delegates actions to the other functions in the program.

One thing I'd maybe change is to put the functions called by the controller above this function, in the order in which they are used. It's not a hard-and-fast rule but it does tend to be a convention that most functional programs follow. It is counterintuitive (or at least I found it so when I started programming) if you are used to laying out documents, because you might want the program to execute starting at the top and then going down. But it's a worthwhile switch to make for readability of your programs for other developers.

Also, well done implementing error handling, and also using arrow functions and forEach, which we did not officially cover in the module.

joemull · 2024-05-25T10:41:06Z

index.html

+         </circle>
+         <circle cx="1798.2" cy="719.3" id="2">
+         </circle>
+        </svg>


I understand why you pasted this into the HTML. If you'd like a challenge, you could try just using the external svg file. To do so I think you'd need to use JavaScript to load it into the DOM on page load, and then (and only then) execute the main "controller" function in your app. This might help the index.html file be easier to read and edit, with the SVG extracted away.

joemull · 2024-05-25T10:43:28Z

script.js

+    return Object.keys(bookData).find(continent => 
+        Object.values(bookData[continent]).flat().some(book => book.country === country)
+    );
+}


Impressive use of these advanced methods!

joemull · 2024-05-25T10:48:20Z

assets/bookdata.json

Impressive JSON curation skills!

joemull · 2024-05-25T11:00:50Z

index.html

+   <!-- Search bar doesn't work, just a placeholder to visually show what the website would look like  -->
+                    <div class="search-bar">
+                      <input type="text" placeholder="Search the website" id="searhbox">
+                  </div>


I'd maybe have omitted this from this iteration, since the focus is really on the map and I'm not sure what I would be searching, so it's a bit of a distraction.

joemull · 2024-05-25T11:01:17Z

index.html

+        <option value="Horror">Horror</option>
+        <option value="Romance">Romance</option>
+        <option value="Thriller">Thriller</option>
+    </select>


joemull · 2024-05-25T11:01:54Z

index.html

+
+        -->
+        <svg id="svg-map" baseprofile="tiny" fill="#ececec" stroke="black" stroke-linecap="round" stroke-linejoin="round" stroke-width=".2" version="1.2" viewbox="0 0 2000 857" xmlns="http://www.w3.org/2000/svg">
+         <path d="M1383 261.6l1.5 1.8-2.9 0.8-2.4 1.1-5.9 0.8-5.3 1.3-2.4 2.8 1.9 2.7 1.4 3.2-2 2.7 0.8 2.5-0.9 2.3-5.2-0.2 3.1 4.2-3.1 1.7-1.4 3.8 1.1 3.9-1.8 1.8-2.1-0.6-4 0.9-0.2 1.7-4.1 0-2.3 3.7 0.8 5.4-6.6 2.7-3.9-0.6-0.9 1.4-3.4-0.8-5.3 1-9.6-3.3 3.9-5.8-1.1-4.1-4.3-1.1-1.2-4.1-2.7-5.1 1.6-3.5-2.5-1 0.5-4.7 0.6-8 5.9 2.5 3.9-0.9 0.4-2.9 4-0.9 2.6-2-0.2-5.1 4.2-1.3 0.3-2.2 2.9 1.7 1.6 0.2 3 0 4.3 1.4 1.8 0.7 3.4-2 2.1 1.2 0.9-2.9 3.2 0.1 0.6-0.9-0.2-2.6 1.7-2.2 3.3 1.4-0.1 2 1.7 0.3 0.9 5.4 2.7 2.1 1.5-1.4 2.2-0.6 2.5-2.9 3.8 0.5 5.4 0z" id="AF" name="Afghanistan" data-country="Afghanistan">


Really impressive the way you have tied the JSON data and the SVG paths together using a custom data-country attribute.

github-classroom bot and others added 4 commits March 9, 2024 12:59

Setting up GitHub Classroom Feedback

db81000

add deadline

2c4bbb2

Add files via upload

42cd48b

Create static.yml

8b8dd4c

avauga03 temporarily deployed to github-pages March 9, 2024 13:08 — with GitHub Actions Inactive

Add files via upload

1cc15e1

avauga03 temporarily deployed to github-pages April 21, 2024 17:52 — with GitHub Actions Inactive

Delete script.js

3d8f9d4

avauga03 temporarily deployed to github-pages April 21, 2024 19:51 — with GitHub Actions Inactive

Add files via upload

25dd70b

avauga03 temporarily deployed to github-pages April 21, 2024 19:52 — with GitHub Actions Inactive

avauga03 temporarily deployed to github-pages April 21, 2024 19:53 — with GitHub Actions Inactive

Delete assets directory

1928e4e

avauga03 temporarily deployed to github-pages April 21, 2024 19:54 — with GitHub Actions Inactive

Delete index.html

769aa01

avauga03 temporarily deployed to github-pages April 21, 2024 19:54 — with GitHub Actions Inactive

avauga03 added 2 commits April 21, 2024 20:54

Delete script.js

9de88b2

Delete style.css

952fe0d

avauga03 temporarily deployed to github-pages April 21, 2024 19:55 — with GitHub Actions Inactive

Add files via upload

488db9b

avauga03 temporarily deployed to github-pages April 21, 2024 19:55 — with GitHub Actions Inactive

Describe your changes here

ba9b6e1

avauga03 temporarily deployed to github-pages April 21, 2024 20:16 — with GitHub Actions Inactive

new fetch URL

6e345f2

avauga03 temporarily deployed to github-pages April 21, 2024 20:21 — with GitHub Actions Inactive

avauga03 deployed to github-pages April 21, 2024 20:26 — with GitHub Actions View deployment

avauga03 temporarily deployed to github-pages April 21, 2024 20:29 — with GitHub Actions Inactive

avauga03 temporarily deployed to github-pages April 21, 2024 20:29 — with GitHub Pages Inactive

joemull reviewed May 25, 2024

View reviewed changes

avauga03 deployed to github-pages March 24, 2025 12:13 — with GitHub Pages View deployment

Feedback #1

Are you sure you want to change the base?

Feedback #1

Uh oh!

Conversation

github-classroom bot commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joemull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-classroom bot commented Mar 9, 2024 •

edited

Loading