@LuisJimenez19
Submitted
I did not find a way that the icon-view would not affect the opacity, I thought of separating it into two different containers which did work but it looks cooler that way, any comment is appreciated.
Looking to hire developers?
@kenreibman
@LuisJimenez19
Submitted
I did not find a way that the icon-view would not affect the opacity, I thought of separating it into two different containers which did work but it looks cooler that way, any comment is appreciated.
@kenreibman
Posted
Hi @LuisJimenez19 ! Great work on this.
There are a few ways you could approach this issue. I'm going to give you my take on it. Hopefully you can understand, and it would help!
First off, I would change your card__image--active
into an a
(anchor) tag.
I would change that into an anchor tag because it makes it more user-friendly to click on the NFT image. It's a little bit of a pain for some users when you have to click on very specific spots of a card/image to open the link. It's better when you just make the whole container click-able and make the actual button (the eye) a decoration, or a "fake button" as I like to call it. The user will still think the eye is a button.
You don't need an active class in this situation since you're not using JavaScript. I would remove all the hover states you have right now. I would also remove the opacity and background-color on your card__image--active.
Add a position: relative
to your card__image--active, move your background-image from your card__image div to the card__image--active div and make a pseudo element on that. So it would be:
.card__image--active {
... other styling
background-image: url(../images/image-equilibrium.jpg);
position: relative;
}
.card__image--active::after {
content: 'url(image)' /* <-- this is your eye image */
position: absolute;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
opacity: 0;
visibility: hidden;
/* (the top, left, and transform properties centers the eye on the container) */
}
.card__image--active:hover {
opacity: 0.8; /* this lightens the image */
}
.card__image--active:hover::after {
visibility: visible;
opacity: 1;
/* this makes the previously hidden eye icon (opacity: 0) appear when the card__image--active div is on hover by the user and changes it to (opacity: 1) */
}
Basically the logic in this, is that the eye icon is currently hidden indefinitely. Only when the user hover overs the --active container, it will become visible, and at the same time the container behind it has its opacity go down. This works because the pseudo element wont receive the opacity effect and they will work simultaneously.
I hope this helps! I'll be honest, I did not test any of the css when I wrote it. so please get back to me if there are any issues.
Marked as helpful
@DaliaMuj
Submitted
I really enjoyed doing this project! I tried to make it as close to the desired design as I could. Any advise about my code is welcome!
@kenreibman
Posted
Hi @DaliaMuj ! I'm happy that you enjoyed the project, you did a great job!
I have a few things that stand out to me which I hope would benefit you:
The first thing I notice is your background. Most times when you set a background with an image, your image will repeat indefinitely. Sometimes it's great. However in this challenge, to match the original design, you only need one background image.
In order to prevent that, in your css, give the body
where the background-image
is being set, another line called: background-repeat: no-repeat
.
Then give it another line background-color:
and set the color to the slightly lighter blue that the ReadMe file gave you, and you've got a very similar looking background to the original!
There are some weird horizontal scrolling in your project which I also fixed by change your height: 100vh
to min-height: 100vh
I would also give your body
a margin: 0
and padding: 0
to reset any default margins and padding that html puts onto your elements.
Usually you want your site's content to be wrapped in a main
element. I would change your div
with the class of container
to a main
element with the class of container
to make it semantically correct. <main class="container"></main>
The two buttons - "Proceed to Payment" and "Cancel Order" are currently input
tags which are strictly used for form fields, like when you want a user to input their email/password. In this case, you wouldn't want that here. Instead I would use an a
tag (anchor tag) and style them. You did it correctly for the "Change" button. Anchor tags are also more "style-able" if you give it a display: block
as well.
Also remember to style your :hover and :focus states for those two buttons to visually show that it is interactable.
I hope this helps, great job! Keep it up!
@AmritPal91
Submitted
Please suggest how can i improve?
@kenreibman
Posted
Hi @AmritPal91 ! Great work :) Your site is nicely done using semantic elements!
Here are some suggestions that I have:
If you are serious in learning web development, I recommend making this site again, or choosing a very similar challenge on here using Flexbox.
I hope this helps! I can tell you're on the right track, you just need the right tools :)
@luis08201
Submitted
Hi everyone.
I remade this challenge, but now using SCSS.
Feel free to give any feedback.
Happy coding :D.
@kenreibman
Posted
Hey @luis08201 ! Great submission. I love how you pushed yourself in using SCSS. Some suggestions from an industry standpoint:
Check your accessibility report that Frontend Mentor offers you, you have quite a few accessibility and HTML issues.
Your BEM naming is a little off. You're giving modifier classes too often; Usually to every third word in your element class name. the double hyphen --
are for modifier events like btn__large--active
or photo__img--highlighted
in your case, header--nav
or --link
should be __link
or main-header__nav
simply put, you should rarely have to use --
I see you're using section
elements which is a great step into a well-structured page. However, you're lacking a main
element that surrounds those section
tags to make it semantically correct. You could wrap all your content inside the body
tag with a main
element.
I also suggest you put a nice descriptive alternative text alt=""
whenever you use an image for accessibility purposes. Same goes for your anchor links as well. I see you're getting a lot of those errors in your footer.
If your image is purely decoration and you absolutely believe that it doesn't need descriptive text for accessibility, you can hide that from screen readers by putting an aria-hidden="true"
in your img
tag. You can read more about that here: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-hidden
I hope this helps, you're doing great!
Marked as helpful
@AnazAnoiar69
Submitted
Hi need advice for responsive font size 🙏
@kenreibman
Posted
Professionally and personally I use the clamp() function for truly responsive typography. This will genuinely take your responsive game up to a professional level.
Check out this video: https://youtu.be/U9VF-4euyRo
@dannyguzman31
Submitted
This is my first submission, please let me know what I can fix. I did struggle a little with the fonts.
@kenreibman
Posted
Nice job on the submission! Unfortunately the preview image isn't appearing on here, but I looked at your live site and it looks great.
I noticed you were centering the div
twice with your body
and container
. Your body
could have been your container
in this case, and the container
could have been your items
. In my opinion it was not necessary to do that.
I would also recommend when you're centering a div
to use min-height: 100vh
instead of height: 100vh
to make responsiveness easier in future projects. Setting a fixed height
like that may bring some issues in bigger projects. I would also stray away from setting a fixed width
like 100vw
, as well, for parent elements. The set width
also applies to containers, or cards as well. I would set a max-width
instead, which is more responsive friendly, and you can also reassign the dimensions in a media query when the screen gets bigger/smaller.
As you do bigger projects on here, you should also start giving your elements more "meaningful" names. Always think about someone else reviewing your work, and wondering if they would be able to understand what each line is referring to. If it was a bigger project, I would have no idea what p1
and p2
is referencing. Start using naming conventions like BEM early, and maybe start putting comments in your code as well, which will definitely help people in this community when they review your code.
I hope this helped!
Marked as helpful
@zastar23
Submitted
It has been a while since my last challenge on this platform, I try my best not to slack off to much. If anyone can tell me how to make the pop-up stay in the same place for big screens I would appreciate it.
Feedback is always welcome!
@kenreibman
Posted
Hey! Great submission!!
Instead of having share__options
as a child element of your share__section
, I would have it as a child element of your main
content. That way your position absolute would be within that container, making it easier for you have the same position.
I hope this helps!
Marked as helpful
@Terak-10
Submitted
@kenreibman
Posted
Hey! Great submission.
I was wondering why you built the page entirely using flexbox
. Did you want to practice flexbox
? Are you more comfortable with flexbox
? I feel like you could have a lot less lines of HTML and CSS if you use grid
.
font-family
as well.Regardless, the design looks great, and you did a great job making it responsive!
@misiucodes
Submitted
Would love feedback on:
@kenreibman
Posted
I forgot to add on... for your JavaScript,
instead of changing the innerHTML, I would create a class that reveals the error text like,
.show-error
with display properties,
and add the show-error
class to the element with .classList.add('show-error')
changing the innerHTML
is not the most correct way to do it, as well as setting the opacity
to 1 and 0 does not hide it for people with screen readers so they will still get an error message regardless of if you can see it on the screen or not.
You can take a look at my code here as well.
Marked as helpful
@misiucodes
Submitted
Would love feedback on:
@kenreibman
Posted
Hey @misiucodes! Nice job! Since you asked a few questions in your description, I'll give you some feedback.
could I have done anything different to make the CSS file cleaner?
heading
, p
, etc. since people looking over your code may not know exactly what p
you are referring to, when there are multiple paragraph elements in the document.~1000px
for this project. A tablet user would have a hard time seeing their input as the form field becomes too narrow in width. That leads me to the next one,max-width
in rem
units for your main container, wrapper
in your case. It helps with responsiveness, and you can always set a new max-width for your container in a media query.Also look at your reports and try to fix some of your accessibility issues as well.
aria-hidden= "true"
main
tag. Your case you could just change your wrapper
div into a main class="wrapper"
.I hope this helps!
Marked as helpful
@bacayo
Submitted
@kenreibman
Posted
Great job! The style looks great.
I would say the only issue with your component challenge here is that your share button is improperly functioning. I am also going to suggest better js code for the share button.
style.display
on each element, it would be better if you toggled your classes.style.css
I would make a class called .show
or .active
or any name that indicates an active state with a display: flex;
property.click
event listener for your shareBtn
shareBtn.addEventlistener('click', () => {
social.classList.toggle('active')
});
This would also, I believe fix the fact that your social button disappears, denying the user from closing the social pop-up as well.
I hope this helps! Please let me know if you have any questions.
Marked as helpful
@audinastgg
Submitted
Here my solution for huddle landing page with a single introductory section, please give some feedback, thank you
@kenreibman
Posted
Good work! Just a few suggestions.
transform: scale(0.8);
on your container
, it's not necessary.max-width
on your container
div as well, since screen resolutions beyond 1440px
gets your content too stretched, then I would center the container
div.I hope this helped, keep it up! You're doing a good job!
Marked as helpful
@peterhannell
Submitted
For this particular project I took a slightly different approach this time with my CSS by setting a base font size on the body and scaling everything relative to that by using ems. I enjoy experimenting with the various ways that CSS that can be used to accomplish the same thing.
I should probably have taken more time to structure my code and trim it down, but hopefully it's not too disorganised or confusing!
As always any feedback or suggestions are welcome :-)
@kenreibman
Posted
Great job on the use of proper HTML5 semantics tags, and the use of rems!
Since you already have a good understanding of not using px
and instead using rems
, I'm going to send you this article that might be interesting for you to read.
Although I really recommend you to read it, summarized, the article talks about the benefits of setting the base font-size to 62.5%
which basically makes 1rem = 10px
. Something you were already close to doing!
Now you might see the pattern, and how rems become much easier to handle. If you want a 255px
container, just set it to 25.5rem
.
From experience, I also suggest giving classes to your heading
tags specific names instead of leaving it as h1
so it's easier for someone going over your work to understand what is going on.
I hope this helps, and keep it up!
Marked as helpful
@Rapha445
Submitted
Hi everyone :)
This was quite a challenging one for me. I have a question about Javascript: I wanted to close the pop-up "share" section by clicking anywhere on the page, so I came up with the following Javascript:
html.addEventListener("click", function (e) {
shareLinks.classList.remove("active");
})
And I removed manually the parts of my html that are supposed to open the pop-up like so:
if (e.target !== shareBtn & e.target !== ImgHover & e.target !== shareLinks)
But this can be a lengthy and laborious process. I was wondering if there is a better way to achieve this...
Any solution or any comments more than welcome! Cheers,
@kenreibman
Posted
Sure! Your viewport width at 1440px
currently looks like this
About the fixed height and width, I mean you instead of using height
with set px
I recommend using min-height
with rems
and max-width
for containers.
Marked as helpful
@jdpaige
Submitted
A big struggle (and one that I didn't come to a solution on) was how to position the images such that:
a) they were positioned with respect to the card b) they maintained position relative to each other c) they maintained position in a responsive way d) one of them had overflow and the others did not
Otherwise, fun project and it made me tackle making an accordion without using a framework or JS.
@kenreibman
Posted
Great job! You're so close to the final result, and I commend your hard work and effort :)
On the desktop layout for the left illustration section, this project requires multiple layers that act as a position: relative
, in addition to uses of z-index
, and the illustration ends up being an position: absolute
that doesn't get hidden by the main card container.
It might be hard to see, but take a look at my code or any Youtube video that covers this solution.
Your viewport width at 375px
has the card overflowing. Although I haven't checked your actual code, this is due to you setting fixed heights such as using the height
and width
property. I would recommend you using max-width
for containers and min-height
for your parent body elements to establish a height.
@Bazthos
Submitted
Hello Everyone !
This is my solution for the challenge : Huddle Landing Page
I mainly tried to focus on the responsive design, I'm quite proud of the result even if I notice afterwards that there are still some points that can be improved.
Any constructive advice or tips are good to take
Thank you for your time and happy coding :)
@kenreibman
Posted
Great submission!
I really like your use of grid for the main section.
I think you should start your desktop-to-mobile breakpoint at ~1000px
Since the viewport widths between 1000px
and 700px
look awkward as a tablet user.
Other than that, it looks great. Keep it up!
Marked as helpful
@muben88
Submitted
Hello, Frontend Mentor people! Hope you're doing great! This is my solution for the Sunnyside agency landing page.
Happy to hear any feedback and advice.
@kenreibman
Posted
Great submission!
Your desktop layout looks great, and you did a great job making it responsive as well.
You should add a text-transform: uppercase
to your header
h1
to match the original design.
For your mobile layout, I initially though about limiting the width like you did with a max-width
but I came to a conclusion that it looks a lot better when the content fills the entire mobile viewport width. Although it is your design choice, I believe mobile users would have an easier time reading content if it took their entire width.
Marked as helpful
@d3vcloud
Submitted
Hi folks, here it is my solution for this challenge...so if there is anything that can be improved please let me know. Thanks a lot.
@kenreibman
Posted
Your desktop layout looks great!
Just a few small things.
Keep up the good work!
Marked as helpful
@saumitr
Submitted
This is my first submission on frontend mentor. I have tweaked mostly all the css property. Let me know if I have followed best practices properly. Any other feedback is greatly appreciated.
@kenreibman
Posted
Great job on your first submission!
I noticed that in your index.html
you had
html body {
background-color: hsl(212, 45%, 89%);
}
you can just have in your style.css
body {
background-color: hsl(212, 45%, 89%);
}
I also noticed that you have a .body
in your style.css
as well which is looking for an element with the class="body"
I think you were meaning to select the body
element as a whole and center items within in. In that case, you should have also just done
body {
min-height: 100vh; <--- *NOTE I also changed this to min-height as it's better practice
width: auto;
display: flex;
align-items: center;
}
Marked as helpful
@Rapha445
Submitted
Hi everyone :)
This was quite a challenging one for me. I have a question about Javascript: I wanted to close the pop-up "share" section by clicking anywhere on the page, so I came up with the following Javascript:
html.addEventListener("click", function (e) {
shareLinks.classList.remove("active");
})
And I removed manually the parts of my html that are supposed to open the pop-up like so:
if (e.target !== shareBtn & e.target !== ImgHover & e.target !== shareLinks)
But this can be a lengthy and laborious process. I was wondering if there is a better way to achieve this...
Any solution or any comments more than welcome! Cheers,
@kenreibman
Posted
I'd also like to mention that your desktop layout is broken and improperly styled.
Your card is also off-center so I changed your css to have the card centered.
body {
min-height: 100vh;
display: flex;
flex-direction: column;
justify-content: center;
background-color: hsl(210, 46%, 95%);
}
height
and background-color
from the .main
class.I highly advise you against setting a fixed height
and fixed width
for containers, backgrounds, etc. in your future projects, as this creates responsiveness issues down the line.
I hope this helps!
Marked as helpful
@Rapha445
Submitted
Hi everyone :)
This was quite a challenging one for me. I have a question about Javascript: I wanted to close the pop-up "share" section by clicking anywhere on the page, so I came up with the following Javascript:
html.addEventListener("click", function (e) {
shareLinks.classList.remove("active");
})
And I removed manually the parts of my html that are supposed to open the pop-up like so:
if (e.target !== shareBtn & e.target !== ImgHover & e.target !== shareLinks)
But this can be a lengthy and laborious process. I was wondering if there is a better way to achieve this...
Any solution or any comments more than welcome! Cheers,
@kenreibman
Posted
So, I actually spent the last hour trying to mess around with your code and figure out the solution to your question with vanilla JS.
This little functionality is actually so common, and I've been trying to figure out an efficient way to do this with vanilla JS. I'm going to keep trying and let you know if I figure it out.
Every resource on the internet recommends Bootstrap or jQuery, and I would recommend you use those frameworks in the future which makes those modals an easier task to create.
Marked as helpful
@kenreibman
Posted
Great job! I like your use of jQuery on the accordion.
Initially your design looks great, but your entire card starts to fall apart when the viewport width starts to get smaller. I have a few suggestions...
width
and height
. Giving fixed dimensions like that lead to lots of responsiveness issues. Instead I would start using max-width
, and use rem
units instead of vh
and vw
for setting container dimensions.height
, and let your content expand on its own. Very rarely do I set a specific height for certain containers in my projects. This relates back to your parent element, which is your body
tag in this case. I would set a min-height: 100vh
to make it more responsive.I actually recommend you to take a look at a video of someone completing this challenge on Youtube, or look at my code for the same problem
I hope this helps and keep learning!
Marked as helpful