Feedback #1
Feedback #1
Conversation
![github-classroom[bot]](https://avatars.githubusercontent.com/in/68187?s=60&v=4){kind=small}
…Issue with the size of the images
Initial set up
touch event successful
…r correction and mistake yet
Alert messages
Successfully added all media queries for responsive puzzle
… two puzzle pages
Buttons and new puzzle page
…appy with its positions. It works for drage events but not touch
…utting pieces back. Started adding 3rd puzzle
user is not allowed to add more than one images to a div, adjsuted cs…
added alt text, width, height, read me file
| img3.src = "puzzle1/piece4.png"; | ||
| img3.width = "199"; | ||
| img3.height = "232"; | ||
| img3.alt = "puzzle piece 4" |
There was a problem hiding this comment.
This is good for a simple puzzle, but as you say in the report, it would be good to make the code DRYer (referring to the principle of "don't repeat yourself"). Maybe the best way to do that is to store the data needed to construct the puzzle in a bit of JSON, and then iterate over it to populate the HTML.
By the way, I do like the style of short lines and good descriptive variables, as it is nice and readable.
|
|
||
| Array.from(dropZone).forEach(function(zone){ //Iterating through elements with this class name as DOM returns the elements as an array | ||
| zone.style.border = "1px solid black"; //adding borders to div to indicate users where to put the pieces | ||
| }); |
There was a problem hiding this comment.
Great use of Array.from and forEach
| piece.style.left = `${leftPosition}%`; | ||
| piece.style.top = `${topPosition}%` | ||
| pieceContainer.appendChild(piece); | ||
| }); |
| startButton.style.display = 'flex'; // Showing the start button | ||
| let element = e.target; | ||
| window.location.reload(); // https://www.freecodecamp.org/news/javascript-refresh-page-how-to-reload-a-page-in-js/#:~:text=The%20simplest%20way%20to%20refresh,and%20loading%20the%20latest%20content. | ||
| }); |
There was a problem hiding this comment.
I like the start and reset buttons--they definitely put the user in control of the game and feel nice and intuitive.
| e.stopPropagation(); // ensure that the click event is just on the image not its parent | ||
| if (e.type === 'dragstart') { | ||
| console.log(e); | ||
| e.dataTransfer.setData('text/plain', e.currentTarget.id); //Seeting the data to add id to my target element that is my div and retriving it in drop- source: https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/setData |
There was a problem hiding this comment.
Excellent. This is complicated stuff!
| #puzzle-container{ /*All puzzles*/ | ||
| display: flex; | ||
| flex-direction: row; | ||
| flex-wrap: wrap; |
There was a problem hiding this comment.
I think the best approach for this kind of layout is a CSS grid. It would provide more fine-grained control with less repetitive code.
There was a problem hiding this comment.
This was a lot of work! I recognize that and also the difficulty of getting the display for something like a puzzle to work, with so many possible states that the game could be in. But I do think this CSS is too repetitive and should have been reduced by breaking down the display into elements that could each be coded once (or a few times for things like media queries).
| flex-direction: row; | ||
| flex-wrap: wrap; | ||
| width: 70%; /*For all media queries, I toogled width and height each time until i was visually satisfied*/ | ||
| height: calc(50% - 1vh); /*For all media queries, I toogled width and height each time until i was visually satisfied*/ |
There was a problem hiding this comment.
There are two choices here that may be causing issues: 1) the choice to use percentages in width and height units, and 2) the choice to define both width and height, rather than just one. Together these seem to be causing skewing when the viewport is taller or wider than the width and height at which it was coded.
I suspect it is also making it more difficult to get the pieces to align without gaps.
Did you experiment with using rem or em units (with only x or y specified, not both)? It may be you'd have the same issues with that approach, but if not, it's worth a try.
| flex-direction: row; | ||
| flex-wrap: wrap; | ||
| width: 50%; | ||
| height: 50%; |
There was a problem hiding this comment.
As with the container, I'm curious whether each piece's layout could be improved by defining just width OR height and using em or rem rather than percentages.
|
|
||
| } | ||
|
|
||
| @media (max-width: 767px) and (orientation: landscape){ |
There was a problem hiding this comment.
Great that you are considering portrait vs landscape! Helps with the responsiveness for sure.
👋! 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
mainsince the assignment started. Your teacher can see this too.Notes for teachers
Use this PR to leave feedback. Here are some tips:
mainsince 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”.main. Click a commit to see specific changes.For more information about this pull request, read “Leaving assignment feedback in GitHub”.
Subscribed: @adeledsg