- Is it semantic correct?
- Should I use always class names instead of high level tags?
- Should I import font in HTML or CSS?
- What could I improve in my CSS?
ROCKY BARUA
@Drougnov
All comments
- ROCKY BARUA• 1,090
@Drougnov
Posted
Hello @cnsacramento, great job. The code looks neat and clean. Here are some things you can improve:
- Change the h2 to h1 as the page should contain a level-one heading.
- The box shadow is not working on the
.card
, cause you've only provided the color. You also need to provide the h-offset and v-offset value. You can learn more about box shadow from here
1 - i_d_s_l• 270
@ilvdrskn
Submitted
hello everyone! I have a problem with a modal window that pops up when you click on the menu button: for some reason it does not cover the entire window, but only the head. How can this problem be solved?
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @ilvdrskn, great job on completing this challenge.
As the .modal-menu is an absolute element and its parent .head doesn't have a defined height, it is only taking the head's initial height. To fix this, simply remove the
height: 100%
from the .modal-menu, and it will get the desired full height.Also, try to fix the accessibility warnings. If you need further help on this, feel free to ask or check out my solution. Have a good day :)
Marked as helpful
1 - spacemanOG• 120
@SpacemanOG
Submitted
Hi everyone. I struggled with this one in the following two areas:
- Giving the Hero Image the Purple tint!
- Using the Picture HTML element.
If you have any feedback for me on how I can improve on those areas, I will really appreciate it.
ROCKY BARUA• 1,090@Drougnov
Posted
Hi @SpacemanOG, the design is looking great. Good job.
Use a pseudo-element(
::after
,::before
) to create an overlay on that image container. Like this:.heroImage::after { position: absolute; content: ""; width: 100%; height: 100%; background: hsla(277, 64%, 40%, 0.6); top: 0; left: 0; }
And don't forget to add
position: relative
to theheroImage
.You can remove the 'source media' for the 300px width as the
img
will be shown by default on lower that 1200px width screen.Hope this helps. Have a good day :)
Marked as helpful
0 - Cenk• 90
@cenkderman
Submitted
Frankly, I had some difficulty in responsive design and it took a lot of time. Why do you think I wasted so much time? Please review my code, I would appreciate if you indicate the places you think are missing or unnecessary.
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @cenkderman, great job.
-
The design is looking perfect on the desktop viewport. But the text-content is exceeding the container in mobile view. Avoid using height if not absolutely necessary.
-
The google font link is wrong. In the HTML change your font link to this:
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin> <link href="https://fonts.googleapis.com/css2?family=Fraunces:opsz,[email protected],700&family=Montserrat:wght@500;700&display=swap" rel="stylesheet">```. Or, just simply add this line at the top of your CSS : ```@import url('https://fonts.googleapis.com/css2?family=Fraunces:opsz,[email protected],700&family=Montserrat:wght@500;700&display=swap');```
- Use a default font in the body tag. Ex: Montserrat and use the other one on the needed element. In this way, you can save some lines.
- Although you have beautifully added the images with display none and media queries, the semantic way would be using the
picture
tag. Like this:
<picture>
<source media="(min-width: 650px)" srcset="./images/image-product-desktop.jpg">
<img src="./images/image-product-mobile.jpg" alt="Gabrielle CHANEL's perfume bottle">
</picture>
Don't forget the alt text 😉.
You can check my solution for further information. If you have further questions, feel free to ask. Have a good day.
(edit) Having trouble formatting the comment. I hope you can still understand what I'm trying to say
Marked as helpful
0 -
- Caio Rosa• 20
@caio-rosa
Submitted
I couldn't apply the purple filter to the image. If somebody else knows how to do it, please give me a feedback.
ROCKY BARUA• 1,090@Drougnov
Posted
Hi @caio-rosa, good job on the challenge.
To achieve the purple filter on the image, you can use pseudo-code(
after
orbefore
) on thecontainer
. At first use position relative to the container.:.container { position: relative; z-index: 0; overflow: hidden; }
And add
after
to the container like this:.container::after { position: absolute; content: ""; width: 100%; height: 100%; background: hsla(277, 64%, 40%, 0.6); top: 0; left: 0; }
this should do the trick.
You might wanna work on the accessibility and responsiveness(mobile devices). For further help feel free to comment or view my solution
Anyway, keep up the good work and have a good day : )
Marked as helpful
0 - caiomiyaji• 40
@caiomiyaji
Submitted
Hi, I solved this challenge using HTML and CSS (flexbox). Any feedback is highly appreciated!
ROCKY BARUA• 1,090@Drougnov
Posted
Hey @caiomiyaji, good work. The card looks awesome.
You can fix the accessibility issue by changing the
h2
toh1
. It is the best practice to have a level 1 header for a well-designed page.Here is a blog about the details if you wanna know more.
Marked as helpful
1 - Makram M Ibrahim• 130
@pharaohmak
Submitted
I would appreciate any tips and advice to help improve my code
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @pharaohmak, good job on the design. Still, you can make a lot of improvements to your codes. Here are some suggestions for you:
- Your HTML isn't semantic. To fix that you can use some tags like
header
main
section
footer
instead of div. - The card should be in the center of the page. Add
justify-items: center
on the container to move it to the center. - The background image is not aligned well. To fix it use
hsl(225, 100%, 94%); background-size: contain; background-position-y: -15%; ```
on the body.
- You have only downloaded font-weight 200. That's why the heading isn't bold. Re-download the font or use this @import url("https://fonts.googleapis.com/css2?family=Red+Hat+Display:wght@500;700;900&display=swap"); in your CSS.
Have a good day my friend :)
Marked as helpful
0 - Your HTML isn't semantic. To fix that you can use some tags like
- -_RIkka• 140
@devsimocastles
Submitted
I did this inside a header tag, is that a bad practice?
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @RikkaaDev, great design. As you asked, yes, it is actually a bad practice. We should only put the logo and nav (if needed) inside the
header
. The actual content should be wrapped around themain
tag. If you want to explore more here is a blog about when to use headerThe design looks great but the image looks a little bit small on mobile devices. Use
width:100%
with media queries to give it full width.Again, great job. Have a good day my friend :)
Marked as helpful
0 - kelvin• 160
@devenyam
Submitted
All feedback is welcome.
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @devenyam, great job. The design looks pretty good.
However, you can improve on the responsiveness part(especially on laptop devices).
-
Use
max-width: 320px(any size)
instead ofwidth
. It will look more flexible. You'll not even need a media query then. -
There are some accessibility issues on the Html part. Use
main
,footer
,section
etc. instead ofdiv
. And use at least oneh1
tag on the page. You can change theh2
toh1
.
Overall, great design. Keep it up. Have a good day my friend :)
Marked as helpful
0 -
- Nicolas Gula• 170
@NicolasGula
Submitted
Any suggestions on how I can improve are welcome!
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @NicolasGula, you did a marvelous job on the design. It's perfect.
However, you need to improve your Html code's accessiblity. Change/Add these things:
- Use
main
,footer
,section
,article
etc. instead ofdiv
. - Use responsive unit like
rem
,em
instead ofpx
- You need atleast one heading in your page. You can change the
<p class="title">
toh1 class="title"
Overall, the design looks great. Keep it up. Have a good day, my friend :)
Marked as helpful
1 - Use
- Ravi Pratap Singh• 110
@cyberspatial
Submitted
I had tried my best to design using the images provided . Could anyone please help me in designing the background more accurately and precisely of this challenge?
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @cyberspatial, great job. You can add the patterns by using pseudo-classes(
:: before
,:: after
) to the body. Try this:body{ position:relative } body::before { background: url(./images/bg-pattern-top.svg) no-repeat bottom right; top: 0; left: 0; } body::after { background: url(./images/bg-pattern-bottom.svg) no-repeat top left; top: 100%; left: 100%; }
It's probably the most efficient way. Also here are something you might wanna consider to change/add:
- The card looks much wider than the actual design. So add
max-width
to the card to fix that. - You need to start with level one heading(h1). That means you cannot start with lower(h2,h3, etc). Don't use the
section
orarticle
tag if there is no child heading. Use div instead.
Overall your design looks great. Have a great day :)
Marked as helpful
2 - The card looks much wider than the actual design. So add
- PMalick Roungou• 270
@mroungou
Submitted
Hi, this is my second challenge on Front-end Mentor! Overall I think my implementation is close to the design. I wasn't able to add the overlay to the button nor "change" one you hovered on them. I'd appreciate a few pointers as to how I could implement that.
Also, though my code works, I'm sure it is not the most efficient/best way to go about things. I would also appreciate a few comments as to how I could make it better. Thanks ;)
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @mroungou, Great job on the implementation.
Although, here are some things you can improve.
- Your hover effect works fine both on the change link and the button but you need to change the color too. So add
color: hsl(245, 75%, 62%);
andbackground-color: hsl(245, 75%, 62%);
respectively in the hover rules. - You need to add a background pattern as the design required. Use
background: url(./images/pattern-background-desktop.svg) no-repeat, hsl(225, 100%, 94%);
- Use
max-width
instead ofwidth
and rem instead of px for responsiveness. Then you wouldn't even need media queries for the card. - To fix accessibility issues, use
header
main
footer
etc. instead ofdiv
. And make sure to use at least one heading on a page. - Change the
h3
toh1
as you need at least one level one heading on a page.
If you need more information or help, feel free to ask or check out my solution. Have a good day my friend :)
Marked as helpful
1 - Your hover effect works fine both on the change link and the button but you need to change the color too. So add
- Dom B• 190
@dombrga
Submitted
This is a mobile-first approach. Any comments about how I implemented this and ways for improvement is highly appreciated. Also on the error message, if any has a suggestion or what is their own way of doing this, feel free to share as well. Thank you!
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @dombrga, Great job. I like how you added different error messages for empty and invalid emails. And good job on the responsive part too.
However here are something you might wanna consider:
-
The Facebook icon doesn't match the size of the other icons. Add some padding to it.
-
To fix accessibility issues, add
aria-label='something like this'
to thosea
tags. -
To fix Html issues, use div instead of section. Cause you need headings while using section or article tags.
Overall, it's looking great. And it's my favorite solution to this challenge. Have a good day :)
0 -
- 👾 Ekaterine Mitagvaria 👾• 7,640
@catherineisonline
Submitted
Hello, Frontend Mentor community! This is my solution to the Huddle landing page with a single introductory section.
I have read all the feedback on this project and improved my code. Due to the fact that I published this project very long ago, I am no longer updating it and changing its status to Public Archive on my Github.
You are still free to download or use the code for reference in your projects but I longer update it or accept any feedback.
Thank you
ROCKY BARUA• 1,090@Drougnov
Posted
Hi @catherineisonline, great job with the layout and responsiveness.
However, here's something you may consider:
-The background pattern isn't flexible enough. Use
background-size: cover
to fix it. -The Facebook icon doesn't look good. Add some padding to both sides.Overall, great solution. Happy coding :)
1 - Patric Gekoski• 30
@Pat-Gekoski
Submitted
I couldn't decide if I should let the content dictate the size of the card or if I should hard code it. I decided to hard-code the width and then use padding to account for the height.
Still a little confused though....I added padding to the main container that the card sits in and even though I hard-coded the width, it got smaller as the viewport shrank....Does the padding force this?
ROCKY BARUA• 1,090@Drougnov
Posted
Hello, @Pat-Gekoski great work.
Just use
max-width:111rem
instead ofwidth:111rem
. It will make the card shrink as the screen width shrinks.Marked as helpful
0 - Ayush Verma• 80
@ayushv45
Submitted
Suggestions to improve are most welcome.
ROCKY BARUA• 1,090@Drougnov
Posted
Hello @ayushv45, Great job. It's looking good.
However, here are something you can do to make it even better:
-
The pattern doesn't look good in 375px or lower width devices. Use a different background image for mobile devices using media queries. It is provided in the images folder.
-
Use the blue-ish color on the headings as the design requires.
-
Try to avoid
div
whenever you can. Usemain
directly on the container class and thefooter
on the attribution class. It's better for accessibility.
Good luck with the next solution and Happy coding :)
Marked as helpful
1 -
- Dom B• 190
@dombrga
Submitted
Is my solution close enough for the challenge? Thank you!
ROCKY BARUA• 1,090@Drougnov
Posted
@dombrga yes sir, it is close enough. You did a marvelous job.
Everything is perfect except the background pattern. It's not flexible enough for bigger screen. You can fix it by adding
background-size: contain
.Change the h2 to h1 to make your code accessible.
Plus you need to add a different pattern for mobile devices. But the design is pretty good. Great job. Happy coding :)
0 - Sachin Shelke• 190
@SachinShelke7
Submitted
I Completed this challenge,If You have any suggestions please give it in feedback. I added hover color shadow also check it on live(preview) site.
ROCKY BARUA• 1,090@Drougnov
Posted
Great job @SachinShelke7. It looks wonderful.
Just lower the opacity of the hover color a little bit.
1 - Herson• 220
@Hersonmei
Submitted
Hi,
This is my first challenge that it took me a while to build and get to finish.
I'm a beginner and I don't have much knowledge about good practices. If you can evaluate my code and give you some initial good practice tips I would be very grateful.
I also had difficulty in the font-weight of the title that didn't match the model despite having bold. How can I fix this?
Thank you very much!
ROCKY BARUA• 1,090@Drougnov
Posted
Hello, @Hersonmei Good job with the layout.
I think you selected only 'font-weight=300' from the google font. I'm not sure though. You may check that out.
Marked as helpful
2 - Sachin Shelke• 190
@SachinShelke7
Submitted
I Completed this challenge,if you have any suggestions drop it down in feedback section.
ROCKY BARUA• 1,090@Drougnov
Posted
Hello, @SachinShelke7 great job.👏
You can make it perfect by merging the container in the middle of the screen. And the design doesn't look good on laptop devices. So you might wanna change that.
But still, you did a great job. Have a good day and happy coding :)
Marked as helpful
0 - arash• 280
@deadazix
Submitted
i cant set background like what challenge asked :(
ROCKY BARUA• 1,090@Drougnov
Posted
Hello, @deadazix good job.
To set those patterns you can use pseudo-elements like
before
,after
. Use this code:body::before { background: url(./images/bg-pattern-top.svg) no-repeat bottom right; top: 0; left: 0; } body::after { background: url(./images/bg-pattern-bottom.svg) no-repeat top left; top: 100%; left: 100%; }
And some other things:
- To make it accessible try not to use the
div
tag as much as you can. You can use themain
tag to the container. - Try to implement the design as accurately as possible. Such as using the font family, font size, etc. as they asked to use.
Happy coding and have a good day. ♥
Marked as helpful
1 - To make it accessible try not to use the
- Tushar Biswas• 4,100
@itush
Submitted
I enjoyed the challenge very much.
I'd really appreciate if you could answer the following:
- What did I do wrong?
- What did I do right?
- How can I improve?
Thanks in advance :)
ROCKY BARUA• 1,090@Drougnov
Posted
Hello, @itush. Nicely done. Here are something you may reconsider:
-
To make your HTML accessible don't use unnecessary divs. You can use
main
,section
,article
etc. instead of div. -
Your background pattern doesn't work well on the big screen. You can use
background-size: contain
to make it fit the width. -
On the card item the box-shadow color doesn't fit well. Try using black color with low opacity.
Happy coding and have a nice day.❤
2 - Kamasah-Dickson• 5,610
@Kamasah-Dickson
Submitted
Hi guys I really need your opinions on this huddle page. I know the footer Is not working well though
ROCKY BARUA• 1,090@Drougnov
Posted
Hello Kamasah👋, great job with the nav and header 👏.
The main section is also good but you need to add some padding on the left of the main coloumns as the design requires.
The footer logo isn't visible due to its color. To make it white, you can use "filter: invert(100%) sepia(0%) saturate(0%) hue-rotate(308deg) brightness(108%) contrast(108%)" on the image.
And about the footer section, the flexbox isn't that flexible cause you have to add width to every row. You can try grid if you want. Here is my solution. It isn't that great but it might help you out a bit.
Happy coding❤
0 - ROCKY BARUA• 1,090
@Drougnov
Submitted
Would love any kind of feedback or suggestion. Especially if there is any better way to do it then please let me know. Thanks in advance.
ROCKY BARUA• 1,090@Drougnov
Posted
Thank you so much. Gonna add it now.
0