Skip to content
OverviewSolutions
17
Comments
71
Ken
@kenreibman

All comments

  • Ken•935
    @kenreibmanPosted over 2 years ago

    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) */
}
    • Then set a hover state on your card__image--active and your card__image--active::after (pseudo element)
    .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
    0
  • Dalia Muj•40
    @DaliaMujSubmitted over 2 years ago

    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!

    Order Summary Component

    1
    Ken•935
    @kenreibmanPosted over 2 years ago

    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!

    0
  • Ken•935
    @kenreibmanPosted over 2 years ago

    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 :)

    0
  • Luis•970
    @luis08201Submitted over 2 years ago

    Hi everyone.

    I remade this challenge, but now using SCSS.

    Feel free to give any feedback.

    Happy coding :D.

    Manage landing page

    2
    Ken•935
    @kenreibmanPosted over 2 years ago

    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
    1
  • Ken•935
    @kenreibmanPosted over 2 years ago

    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

    0
  • Ken•935
    @kenreibmanPosted almost 3 years ago

    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
    0
  • Florin.Porut•380
    @zastar23Submitted almost 3 years ago

    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!

    Responsive article preview with flex and JS

    2
    Ken•935
    @kenreibmanPosted almost 3 years ago

    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
    1
  • Ken•935
    @kenreibmanPosted almost 3 years ago

    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.

    • Some of the paragraphs and sub-heading are the wrong font-family as well.
    • I don't think you included a function mobile menu as well, which I would have loved to see.

    Regardless, the design looks great, and you did a great job making it responsive!

    0
  • Misiu•50
    @misiucodesSubmitted almost 3 years ago

    Would love feedback on:

    • CSS styling: could I have done anything different to make the CSS file cleaner?
    • CSS styling: is this optimized for mobile and different screen sizes?
    • Javascript: could I have done anything to factor my code differently so it's cleaner and less repetitive?
    • for the form, is there anyway to maintain the height of the form after the error messages appear?

    Intro component with signup form

    3
    Ken•935
    @kenreibmanPosted almost 3 years ago

    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
    0
  • Misiu•50
    @misiucodesSubmitted almost 3 years ago

    Would love feedback on:

    • CSS styling: could I have done anything different to make the CSS file cleaner?
    • CSS styling: is this optimized for mobile and different screen sizes?
    • Javascript: could I have done anything to factor my code differently so it's cleaner and less repetitive?
    • for the form, is there anyway to maintain the height of the form after the error messages appear?

    Intro component with signup form

    3
    Ken•935
    @kenreibmanPosted almost 3 years ago

    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?

    • People may disagree, but I like to name all of my HTML elements, including 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.
    • is this optimized for mobile and different screen sizes?*
    • Your media query breakpoint is way too late. I would switch from the desktop to tablet/mobile view at ~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,
    • Set a 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.

    • The icon-error just needs an aria-hidden= "true"
    • Also using correct HTML5 semantics, you should always wrap your content in a main tag. Your case you could just change your wrapper div into a main class="wrapper".

    I hope this helps!

    Marked as helpful
    0
  • batuhan•60
    @bacayoSubmitted almost 3 years ago
    • Desktop active design was challenging. I am not sure it was the optimal way to implement active state.

    article preview component

    1
    Ken•935
    @kenreibmanPosted almost 3 years ago

    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.

    • Instead of adding a style.display on each element, it would be better if you toggled your classes.
    • In your style.css I would make a class called .show or .active or any name that indicates an active state with a display: flex; property.
    • Then make a 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
    0
  • Ken•935
    @kenreibmanPosted almost 3 years ago

    Hey! Awesome design. Not sure why the images aren't showing up on the preview, but I saw your live site and it looks great. Amazing job with the responsive design as well.

    1