Pop Modal #11
Pop Modal #11
Conversation
Form Validation
There was a problem hiding this comment.
Hi @41hulk,
Good job so far!
To Highlight 👏
- There are no linter errors 💯
- When the user clicks (or taps) the button to check project details, the popup with details about the project appears. 👍
- When the user clicks (or taps) the close (X) button, the popup disappears. 👍
- The popup is responsive, it fits mobile and desktop sizes properly. 🥇
There are some issues that you still need to work on to go to the next project but you are almost there!
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
You can also consider:
- N/A
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
index.html
Outdated
| <!-- <div id="modal"> | ||
| <div id="modal-snapshot"> | ||
| <div id="modal-cancel"> | ||
| <i onclick=closeModal() class="fa fa-times" title="medium" aria-hidden="true"></i> | ||
| </div> | ||
| <img id="screenshot" src="./image/pic.jpeg" alt="project snapshot"> |
There was a problem hiding this comment.
- Please avoid keeping commented code in your codebase because it does not look clean.
|
|
||
| button.addEventListener('click', () => { | ||
| modal.style.display = 'block'; | ||
| const content = `<div id="modal"> |
There was a problem hiding this comment.
- I recommend sticking the project design as much as possible because I think this is one of the crucial aspects of becoming a professional developer nowadays. Please, take a look at the original and your design below and fix the differences 👇
- Original mobile
- Your mobile design
- Original desktop version
- Your desktop design
I have marked the differences with blue color, kindly use similar colors for your close buttons and make yoursee liveandsee sourceinputs larger in size.
brytebee
left a comment
brytebee
left a comment
There was a problem hiding this comment.
Hi Team 👋,
Great work on making the changes 👏 👏
You've done well implementing some of the requested changes requested by the previous review, but there are still some which aren't addressed yet.
Suggested changes
Check the comments under the review.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
|
|
||
| button.addEventListener('click', () => { | ||
| modal.style.display = 'block'; | ||
| const content = `<div id="modal"> |
There was a problem hiding this comment.
- Required: There is yet some alignment of the button left to have a similar design as your chosen Figma Template.
- Kindly align the tags to left
- Also justify the buttons as shown in the images attached.
| Your Design | Figma Template |
|---|---|
 |
 |
- Required: Moreso, on the desktop popup
- The button text and image should match the template.
- The bottom should be aligned right as shown in the attached image.
| Your Design | Figma Template |
|---|---|
 |
 |
form.js
Outdated
| @@ -0,0 +1,18 @@ | |||
| function validateEmail(form) { | |||
| // eslint-disable-next-line no-underscore-dangle | |||
There was a problem hiding this comment.
- Optional: Ensure you do not disable
ESlintin your chosen solution. This ensures there is conformity in the codebase in your team here at Microverse and in future jobs.
| card.append(img, desc); | ||
| projectsDesktop.appendChild(card); | ||
| } | ||
| // eslint-disable-next-line no-unused-vars |
There was a problem hiding this comment.
- Optional: Remove
ESlintdisable from your code as discussed earlier.
There was a problem hiding this comment.
I used this because this function is used in a string and seems like it's not being used
t-yanick
left a comment
t-yanick
left a comment
There was a problem hiding this comment.
Hi @41hulk,
Your project is complete! There is nothing else to say other than... it's time to merge it 
Congratulations! 🎉
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
What does this PR do?