Looking to hire developers?
@zineb-Bou
@besttlookk
Submitted
I have tried to follow all the best practices. If you have suggestions do let me know.
Feedbacks are always appreciated.
Can anyone tell why in the screenshot not everything is visible when everything is okay. Same thing has happened in my previous solution.
Thank You,
#happyCoding
@zineb-Bou
Posted
Your solution looks good I like the animation, here are some notes to consider.
The images are stretching when scaling down to small screen sizes, using the CSS property background-size: cover; will help you in this scenario.
Some interactive elements such as the menu button need to be rapped inside a <button> tag, choose the right semantic HTML element.
Happy coding.
@DanteBenicio
Submitted
I liked the development process of this project, the development of this Landing Page was very satisfactory.
send me feedback
@zineb-Bou
Posted
Hi there, your solution looks good, is nicely responsive and I love the animation on the header, here are some suggestions concerning accessibility.
aria-hidden=” true”<button> instead of a <div>.Good luck
Marked as helpful
@Alexuva
Submitted
This is a temporal solution for the JS of the accordeon
@zineb-Bou
Posted
Hi there, since this is a temporal solution I am dropping some comments that may help you to improve your solution next time.
On mobile screens, the text is overlapping, I think setting an explicit height may cause this kind of problem, so let the container shrink according to the text size.
Anything that is clickable or has a hover effect means it’s an interactive element, the toggle arrow actually it’s a button, so you can wrap the <img> inside a <button aria-label=” toggle arrow”>.
Illustration images are for decoration purposes, there is no information to convey so it's better to hide them from the screen readers using aria-hidden=” true”.
<body> is not taking the total height when scaling down to mobile screens, I would recommend using min-height:100vh instead of heigh:100vh to avoid this problem.
Good luck
Marked as helpful
@SiegeTank-90
Submitted
Thanks to F.E.M. for putting up the site and setting up these free challenges. Feed back is welcome on any parts, but in particular I'm looking for anything, that would be more likely done in a working environment, if this had been done to work with a team of developers what kind of change would have been made?
@zineb-Bou
Posted
Hi there, congratulation on completing this challenge, taking a quick look at your solution I think there are a couple of things that need some improvement
<h5> inside a <div>, <ul> is more adequate for that.[<h1> ..... <h6>] are used to construct a table of contents for a document, so avoid using heading elements to resize text. Instead, use the CSS font-size property.Happy coding
Marked as helpful
@VarnerDamascenoJr
Submitted
My first difficulty was align the dice and circle in the middle of the card. Then I learned two different ways to align content. The first one using grid/flex and the another was using margin( left/right) and the position.
@zineb-Bou
Posted
Hi there, congratulation for completing your first challenge here 🎉, keep going, you gonna learn a lot by completing different challenges 💪
<button> tag. Take the habit of using the right HTML semantic elements for the right purpose.Good luck.
Marked as helpful
@lewisoutdoorllc
Submitted
Light and dark mode while trying to keeping the users choice was fun to figure out so that the app would keep the users selection by using local storage
@zineb-Bou
Posted
Hi there, Good job on this challenge, I am dropping some comments that may help you to improve your solution even further.
class=” App” is not taking the full-screen height, so instead of writing height:100vh, min-height:100vh will do the job.<div>, use <button> with an aria-label=” toggle theme”, since there is no textual indication of the button purpose, we still need to provide alternative text for users who use assistive technology, (the same thing applied to the delete button and the checkbox)Happy coding
@delfigica
Submitted
@zineb-Bou
Posted
Hi there, Good job on this challenge, your solution is nicely responsive, semantically looks good, I loved that you have used the <blockquote> tag to wrap the advice.
aria-label to the dice button so it will be recognized for those who are using screen readers.Happy coding
@bhonePyae99
Submitted
@zineb-Bou
Posted
Hi there, Good job on this challenge, your solution is nicely responsive, though there are some cards that have different sizes compared to the others on mobile screens.
Here are comments about semantics and accessibility
<h2> here, the title “where in the world” and “dark mode” the theme button, first of all, button text shouldn’t be a title, in this case, it is better to use <h1> where in the world</h1> since it is a single-page website.<div> for the theme button use a <button>.alt text to the flag images since it is an informative image.“fitter by region”.Happy coding
Marked as helpful
@EricL81
Submitted
@zineb-Bou
Posted
Hi there,
<button> rather than an <img>, try to choose the right HTML semantic elements.Happy coding.
@khalil227
Submitted
@zineb-Bou
Posted
Hi there, Good job on this challenge I would love to drop some comments that may help you to improve your coding skills even further
<div> to most of your elements, for instance instead of using a <div> for the dice button use a <button> because it’s an interactive element.<h1> every website need to have an <h1> title, in this case, it is a bit subtler, so you can choose <h1> Advice generator</h1> then make it sr-only (screen reader only ) since there is no <h1> in the design .Good luck
@ZybartasRingys
Submitted
My solution to this challenge, I would appreciate your feedback how to improve my JS.
@zineb-Bou
Posted
Hi there, your solution looks good I am dropping some suggestions that may help you to improve your code even further.
1- Concerning the images, since they are for decoration don't keep them visible for the screen reader(we don't need it to read their source path).
2-The advice text instead of putting inside a <div> it's better to put inside a <p> tag.
3-Add an aria-label=" generate advice", to make the button readable by the screen reader.
Happy coding :)
Marked as helpful
@zineb-Bou
Submitted
Any suggestions are welcomed 🙏.
@zineb-Bou
Posted
Hi, thank you for your detailed review, I rarely get one.
I will make sure to take everything you have mentioned into consideration 😃.
@Md-Raihan-Alam
Submitted
Any suggestion for update is welcome
@zineb-Bou
Posted
Hi there, here are some suggestions that may help you to improve your code even further.
1- The card doesn't fit all the screen sizes, 590px <scren-size<700px the pattern divider gets overflow the card, your solution should be responsive.
2-The get request needs to be done when clicking on the dice button, you can be done using only the effect hook.
3- For accessibility purposes, you need to use the right HTML semantic element, so instead of using <dev> for the dice button use <button>. you also need to hide any images from the screen readers that have a decoration purpose.
Good luck 😃
@RUTVIK-SANATHARA
Submitted
Hello everyone! this is very easy task so please give your suggestion for my coding skill and how to improve! thank you.
@zineb-Bou
Posted
Hi there
Some suggestions that may help you to improve your code even further
Give the image an explicit width and height along with using object-fit property to keep the image look consistent when scaling down to different screen sizes, avoid using overflow: hidden it's clipping the image content.
The illustration image is not centered horizontally on the mobile screen.
Instead of wrapping the question inside the button, give it the right HTML markup, for instance, you can use <h3> ( the <h2> should be used for the card title and <h1> for the page title), give the button a meaningful text that describes exactly what it does(toggle arrow button), then hide the text visually since there is no text provided in the design.
-You are submitting a solution for a completely different challenge.
@bajajharsha
Submitted
Hey Guys Pease let me know if you have any feedback that can make it look better.
@zineb-Bou
Posted
Hi, there your solution looks good,
some suggestions that may be helpful:
You can use the <picture/> tag for the card image, this will allow you to set different images based on the screen viewport instead of using <div/>
Since the image is just purely decorative and there is no alt provided, use aria-hidden: true to hide it from the screen reader.
The same thing applied to the icons, we don't want the screen reader to read it since it's purely decorative.
You need to add the alt for the avatar image(it's an informative image), the screen reader will read the image source path if we don't add any alt text.
Don't use <hr> for decoration purposes, it's used for indicating if there is a transition between headers and paragraphs, you can use only CSS to draw the line using the <div> border and play with card padding to make the left and right space.
Marked as helpful
@ThutaZaw2603
Submitted
This is the first challenge that I do from frontend mentor. Unfortunately, my site would not be responsive on tablets from 800px to 430px viewports. Can pay feedback and tell me any problem you see on my site. Thank You !
@zineb-Bou
Posted
Hi there,
Using only @media screen and (max-width:428px) you are not covering all screen sizes, which makes the content get smashed together then scaling down to a small screen size
You shouldn't include more than h1, Apply the correct heading level
Avoid setting an explicit Height to element, this won't allow the container to shrink when content grows inside it.
When the image is just for decoration hide it from the screen readers using aria-hidden: true
I dropped some notes after I checked your code quickly.
Happy coding.
Marked as helpful
@realMecoy
Submitted
Hey guys and girls, please tell me how you positioned the background the same as the jpgs in the design file, I couldn't figure that out, thanks in advance.
@zineb-Bou
Posted
Hi there,
Check your GitHub repo link, I wasn't able to access it.
@rhythmsaha
Submitted
suggestions ?
@zineb-Bou
Posted
Your site looks good on different screen sizes.
Overall semantically also looks good, just be careful you don't need to include more than <h1/>. web page has only one <h1/>.
Add text to social media links and make it sr-only, visually hidden, and read-only by screen readers. hide the icons because we don't want the screen reader to read the image source path, the same thing applied to the hamburger menu.
Footer nav, I think they are all links, not just list items.
Any image that is purely decorative hides it from the screen reader using aria-hidden: true.
Happy coding
@milen-nenkov
Submitted
Hi,
Is it me or layout challenges get easier the more you do? :) This is the first time I would scale up to desktop size instead of scaling down to mobile.
I would appreciate any tips or comments you have about my code.
Regards Milen
@zineb-Bou
Posted
Hi there,
Be careful there is a horizontal overflow.
Sadly your solution doesn't scale to fit different screen sizes.
Marked as helpful
@dusanlukic404
Submitted
This is my first challenge completed with Vanilla JS. I learned basics of JavaScript and want to practice more working with the DOM. Keep working and learning more JavaScript. It's awesome 😍 Any feedback or suggestion would be very helpful to me ❤️😊🎉
@zineb-Bou
Posted
Your solution looks good on different screen sizes.
Setting h1 as a card title may look normal on this task since the card is the only component on the page, though this card would be part of a page in real production and a website should contain only one h1, so avoid choosing h1 as a card title you could choose h2 instead.
The social media icons are actually links, so you should put <a/>, add text inside it and make it sr-only then hide the SVG icons from the screen reader since they are purely decorative
happy coding 🙂
@saepulmalik27
Submitted
need your feedback, please kindly give me a feedback
@zineb-Bou
Posted
Hi there, here are some notes that may help you to improve your code even better,
-There is a huge horizontal overflow, check the background images I think this is what makes the background have this huge overflow.
-Avoid putting an explicit height, I noticed you are giving your card a height, this won't allow it to shrink to fit the content inside when it grows.
-You can hide the background images from the screen reader since they are just for decoration using aria-hidden: true
@CoolTaher
Submitted
i want to perfect the sizeing ....... learned some grid content ; )
@zineb-Bou
Posted
Hi there, some suggestions that may help you to improve your solution
Your solution doesn't cover different screen sizes, make sure to keep it responsive.
Setting the container to an explicit width, won't allow it to scale down when changing between screens, also the reason why there is a horizontal overflow when changing to small screens, so always avoid setting explicit width or height, the height could be even worst because the container won't ever grow the fit the content inside it.
Avoid using ids to select your CSS elements, using a good naming convention (for instance BEM) you will be able to give your element unique class names.
You are repeating border-radius for imgQuote note that CSS will never consider the first value, will take the last one, make sure to never repeat the same property twice
@allysonbelo
Submitted
My first project using Sass ;)
@zineb-Bou
Posted
Hi there, some notes may be helpful
I recommend using the <h2> inside the card rather than the <h1>, in real production this card would be a component part of the whole website, and the website should contain only one <h1>
You can simply use the <img > tag for the card image no need to use <figure/> either if you want to use some caption to it, at this time it's useful, <figure /> is one of the HTML 5 features before if you want to add a caption to image, illustration or diagram you have to add <p> tag or <sapn>
You have to pay attention to the heading level, using <h2> for just a simple share tag and a <h3> for the username, it is not very helpful when it comes to accessibility, I recommend using just <p>, it's fairly enough.
For the arrow share button, it is actually an icon button, you have to use the <button> with aria-label="share it", and hide the share icon since it's only for decoration. the same thing is applied to social media links they are not images, they are links that should contain hidden text that is read using a screen reader and it leads the user to another page when it gets clicked on.
I notice that you are using a reset.css file, I recommend checking this article, it's the modern CSS rest, I really love it.
I always make sure to mention this, use rem (or em ) instead of px, it's an absolute unit, using rem will make everything scale when the user chooses the base size.
Instead of putting height:100vh, use min-height: 100vh, why? because the layout won't scale to fit the total screen height when changing to small devices.
Also avoid setting the overflow: hidden, for large containers, if there is any content overflow this won't fix the issue, it's a temporary solution just to beautify your design, and imagine if the content will grow inside the card it will be cut off.
I noticed repeating the overflow property two times inside the container, CSS always takes the last one been set, make sure to not repeat the same property two times.
Happy coding
Marked as helpful
@ashrafess00
Submitted
thank you for providing figma file for free and i really enjoyed making the website
@zineb-Bou
Posted
Hi Ashraf,
Your solution looks good, I am dropping some notes that may help you even further.
aria-hidden: true, letting the image be exposed to the screen reader, it will read the image source path, and don't forget to add alt=" page logo"."so you want to travel to space" it's all considered as an <h1>, you can use <span/> to divide the title into two different sections so you will be able to style them differently.<p > for titles use <h2> instead, and your website should only contain one <h1> tagexplore I think it's not meant to be for decoration, it's actually a link that will lead the user to a different page to see more options, so don't make it an <div> it's <a>alt to it, it's helpful for all users, imagine there is a problem in image loading, alt would provide a description about the image, even if the user won't be able to see it.Marked as helpful