Gnandal Elisée• 170
@Gnandal
Submitted
Dirk
@dirkvankrieken
All comments
- Dirk• 210
@dirkvankrieken
Posted
Nice job! Everything looks great. One small thing you forgot I think is to update the email address in the success message, it still says '[email protected]' but it should update to the actual email address submitted through the form.
Marked as helpful
0 - anthony• 170
@Codingtry123
Submitted
What are you most proud of, and what would you do differently next time?
wrap svg inside container simplify the js code
What challenges did you encounter, and how did you overcome them?
how to toggle the share panel add event listened respond to the click event
What specific areas of your project would you like help with?
how do I write media query in js
Dirk• 210@dirkvankrieken
Posted
Hey nice job! Some improvements that could be made: Not all colors match well with the designs (background color and font colors) The spacing between the text blocks (margins/paddings) could be better matching. The share element on small/mobile size could be made so that the size of the containing element does not change on click but remains the same. The answer to your question "how do I write media query in js" is, I think that you can use the window.innerWidth property to check the width of the window in Javascript.
0 - B4GR1N3RD• 110
@B4GR1N3RD
Submitted
Dirk• 210@dirkvankrieken
Posted
Nice job on completing the project! I think your code looks good but I think the end result is a little too small, and the cards with texts are not easily readible. You did not make it responsive which was an important part of the challenge. When resizing to smaller screen, it should change from the desktop design to the mobile design.
0 - ziedlahbib• 80
@ziedlahbib
Submitted
Dirk• 210@dirkvankrieken
Posted
Nice try, but there are some things that should be improved. The link to the your github repository is not working. The fonts are not okay, its showing a serif font instead of the one indicated in the style guide. The sizes of the texts and elements are not matching well to the design. Nice job on using flexbox for the desktop layout of the cards. Next time take a little more time to closely match everything to the design.
0 - P
@Guilhxrme77
Submitted
Dirk• 210@dirkvankrieken
Posted
I cannot check the repository because it says the link does not exist. But I can see your HTML and CSS code through the inspector, in the preview version of the page and it looks good. For the preview I would say the desktop size version looks great! However for the mobile size I think the header image is too big. You mainly see a big picture and not much of the information underneath, unless you scroll down.
Marked as helpful
0 - Vandorpe Jentl• 50
@VandorpKe
Submitted
Dirk• 210@dirkvankrieken
Posted
Nice job! Your project closely matches the design for bigger screens. Unfortunately it is not optimized for smaller screens such as mobile screens. You did account for some optimization using media-queries for a smaller screen but it's not optimal yet. For smaller screens the texts overflow the containers. You could have prevented this issue by adopting a mobile first work-flow where you use media queries for the bigger size screens. Apart from that everything looks good!
Marked as helpful
0 - Mauricio• 120
@Lucknagh
Submitted
What specific areas of your project would you like help with?
I had difficulty centering the text of the links, if you could give me feedback on other ways that I could have aligned I would appreciate it
Dirk• 210@dirkvankrieken
Posted
Nice job, your solution looks great and perfectly matches the design.
Some points about your code that I think might be improved: To my understanding one should use the next heading available so first you use h1 for the main heading, then you use h2 for the second smaller heading, after that h3, etc. I see you use h2, h5 and h6, and that might be because by default those headings font-sizing matches the design font the best. But I would suggest you use h1, h2, and h3. That is, as far as I know considered best practice. But furthermore, instead of using multiple headings element, I would use only h1 for the name (because that is the heading of the element) and something more like a <p> paragraph </p> for the "london, UK" subtext, because that is not really a heading and for the "front end developer and avid reader" text I would use something like a <span>. Or another <p> paragraph, and then give both paragraphs class names to target them separately through your CSS code.
It's always a good idea to update the README.md to something that you wrote yourself. You can use the README-template.md as a template. And then remember to delete the README-template.md file because it doesn't have to be part of the repository.
The .vscode folder with its contents also don't have to be part of the repository A line with .vscode could have been added to the .gitignore file so that you don't accidentally add it.
I think the way you centered the text of the links with
text-align: center;applied to the main element is perfectly fine.Marked as helpful
0 - dsaneshaikh• 40
@dsaneshaikh
Submitted
Dirk• 210@dirkvankrieken
Posted
Nice work! Remember to update the README.md file to something you wrote yourself. You can use the README-template.md file to make it your own.
0 - Avanti-L• 50
@Avanti-L
Submitted
Dirk• 210@dirkvankrieken
Posted
What is good: You used semantic HTML such as a Main and a footer element. The layout looks good on different screen sizes. The code looks is well-structured and readable. The end product closely matches the design.
What could have been improved: You could have updated the README.md so that it shows relevant info that you provided.
Marked as helpful
0
