Skip to content
OverviewSolutions
9
Comments
47
Eugenia
@JaneMoroz

All comments

  • Eugenia•950
    @JaneMoroz

    Hey Zet! Wow, your solution is awesome ❤️

    Some things I've noticed:

    • I think I would just add a bit of top padding to the header or nav
    • And often this kinda of logos are supposed to be links (which lead to Home page).

    About the circle number icon:

    • I think the way I would do it:
    <div class="section-number">01</div>

    // The circle with the number inside
.section-number {
  position: relative;
  height: 40px;
  width: 40px;
  display: flex;
  align-items:center;
  justify-content: center;
  border: 1px solid grey;
  border-radius: 50%;
}

// The line
.section-number::before {
  position: absolute;
  display: block;
  bottom: 100%;
  left: 50%;
  transform: translateX(-50%);
  content: " ";
  height: 70px;
  width: 1px;
  background-color: grey;
}

    This is not styled exactly, just trying to give some general idea.

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hey! Your solution is great 😊

    Frameworks can make your life easier, especially when the project is big. I used to code big projects with Vanilla JS and in the end I kinda had to create some sort of framework myself, without it it's hard to follow DRY principle.

    Some people don't like frameworks for various of reasons. One reason is that they're changing often and you need go though docs again or some dependencies stops working together etc.

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hey! Very nice solution 🎉

    • I guess you got some 'landmark something warnings'. I get them when I forget to wrap the main content of the page into the main tag. So I suggest you to use main tag instead of div with class="main"
    • Instead of h3 I suggest to use h1 and then style it.
    • Maybe it's better to wrap Jules Wyvern into the a instead of p since it seems to be a link and then style it because link tag usually has default underlying etc.

    Keep it up! And good luck 🍀

  • Eugenia•950
    @JaneMoroz

    Hello! Cool solution to the challenge 👍

    I've noticed that the submit button works the first time but not the second. So it seems like the problem is that after we click 'Dismiss message' and go back to the initial form, event listeners are not attached. I think this is because the form we're going back to is not the initial one but just newly created (with function pageSwapPrevious()).

    • So maybe one solution is to make sure that newsletter is not removed when success shows up. To do so you need to give success element position: absolute and switch between display:none and display: something else instead of changing document.body.innerHTML .
    • Another solution is to move subscribeButton.addEventListener logic into onSubmit() function for example, you will also need to add inputElement and invalidMessage:
    function onSubmit() {
  const inputElement = document.querySelector('.js-input');
  const invalidMessage = document.querySelector('.invalid-email');

  const regx = /^([a-zA-Z0-9\._]+)@([a-zA-Z0-9])+.([a-z]+)(.[a-z]+)?$/
  
  if (inputElement.value.match(regx)) {
    pageSwapNext();
    return;
  } else {
    invalidMessage.innerHTML = 'Valid email required'
  } 
}

    And then just add it to the button's onclick: <button onclick="onSubmit()" class="subscribe js-subscribe-button">Subscribe to monthly newsletter</button>

    Keep it up! And good luck 🍀

  • Eugenia•950
    @JaneMoroz

    Hey! Your solution is great 🤩

    The only things I've noticed are some accessibility issues:

    • You can wrap the main content (which is QR Code in this case) into the main tag or replace existing div tag with main.
    • Also usually the page should have h1 tag. You have h2, I guess you decided to use it because it has desirable font-size but you can style h1.

    Overall everything is great and I like that it is responsive ☺️

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hello Richard! I love your solution to this challenge ❤️

    I think I found a solution to the replies width bug. If you remove the default replies and create new reply you'll see that they are not filling the whole horizontal space.

    The reason replies are kinda taking all horizontal space in the second comment (with default replies) is because default replies kinda stretch Replies_repliesContainer so the new replies (children) just fill the container's width.

    So in order to solve this problem you need to make Replies_repliesContainer take the whole width of it's parent. For example you can add width: 100% to Replies_repliesContainer and CommentAndReply_container. Or you can add flex:1 to Replies_repliesContainer, this way Replies_repliesContainer will just take the rest of flex-container space.

    Keep it up! And good luck 🍀

    P.S. You can also use gap property to add the space between the vertical line and the reply.

  • Eugenia•950
    @JaneMoroz

    Hey! You solution is great 👍

    I see that the image is overflowing. I looked at your code and I think that adding overflow-x:hidden to the body might fix it.

    Keep it up! And good luck 🍀

  • Eugenia•950
    @JaneMoroz

    Hey! Very nice solution to the challenge 🎉

    The only thing I've noticed is that the image aspect ration is changing when the screen width increases. You can fix it with object-fit: cover;, I use it quite often.

    Keep it up! And good luck 🍀

  • Eugenia•950
    @JaneMoroz

    Hey Travis! Your solution is great 👍

    I think that

    .faq-card {
  justify-content: center;
}

    is responsible for all this movement. The .questions height is changing when you expand a question/answer section and since it should be in the centre of flex-container it moves up and down.

    I suggest you to:

    • remove absolute positioning from the .card-title, you can use padding or margin to position it.
    • remove justify-content: center; from .faq-card and use gap to add some distance between .card-title and .questions

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hey! Congrats 🎉 This is a great solution to the challenge!

    The only things I've noticed:

    • A little style issue: if the screen's height is less than 730px, background-colour doesn't fill the whole screen
    • Also I can put operators one after another (for example 10+/2) and it gives me an error

    Keep it up! And good luck 🍀

  • Eugenia•950
    @JaneMoroz

    Hey! Your solution to this challenge is great! 💪

    I looked at you code. So I'd advice to remove position:absolute; from profile-picture and margin-bottom: 4rem; from top-background and then just add transform: translateY(-50%); to profile-picture.

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hey! Congrats 🎉 Your solution to this challenge is great! ❤️

    The only things I've noticed:

    • You need to wrap the main content of the page into the <main> tag. It will solve all these landmark issues and improve accessibility.
    • Also when I make it smaller, the card image body is kinda cut in half. You have height: 100vh;so I suggest to it change min-height: 100vh; and maybe add some top and bottom margins to card. This way you card will still be centred for larger screens and not cut from above for smaller screens.

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hi, Cletus! Congrats 🎉 Your solution to this challenge is great! 👍

    The only thing I've noticed:

    • You need to wrap the main content of the page into the <main> tag. It will solve all these landmark issues and improve accessibility.

    Keep it up! And good luck 🍀

  • Eugenia•950
    @JaneMoroz

    Hey! Congrats 🎉 Your solution to this challenge is awesome! ❤️

    The only things I've noticed:

    • You need to wrap the main content of the page into the <main> tag. It will solve all these landmark issues.
    • You can use object-fit: cover to the img, so it doesn't lose aspect ratio.
    • I have noticed that the original design has grid and images are different sizes, I had similar gallery layout in my other project. Here you can see that I used grid-template-areas and then positioned each image. It might help, if you want to make it more similar to the original design (not a must!).

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hey! Your solution to this challenge is awesome! ❤️

    The only things I've noticed:

    • You need to wrap the main content of the page into the <main> tag. It will solve all these landmark issues.
    • If you use button/link without any text (or just image/icon), I suggest to add aria-label to the <button>/<a> tag to improve accessibility. You can read more about it here. For example:
    <a href="#" aria-label="facebook"><img src="../images/icon-facebook.svg" alt="" class="duration-200 ficon"></a>
    • Also you might want to add cursor: pointer to the links.

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hi, Alexander! Your solution is great! ❤️

    The only thing I've noticed:

    • You need to wrap the main content of the page into the <main> tag. It will solve all these landmark issues and improve accessibility.

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hey! I like your solution! ❤️ The button animation is awesome 🤩

    The only things I've noticed:

    • If you use button/link without any text (or with only image or icon), I suggest to add aria-label to the <button> (or <a>) tag to improve accessibility. You can read more about it here. Also you might want to add aria-hidden tag for the image inside of the button. More about aria-hidden. For example:
    <button aria-label="next advice" class="rounded-full p-5 bg-neonGreen absolute -bottom-[32px]">
        <img aria-hidden="true" src="images/icon-dice.svg" alt="">
 </button>
    • You might want to put <div class="attribution bg-darkBlue text-lightCyan font-manrope"> inside the <main> tag. It will solve this landmark issue and improve accessibility.

    Keep it up! And good luck 🍀

    Marked as helpful
  • Eugenia•950
    @JaneMoroz

    Hi, An! Your solution is great! ❤️💪

    The only things I've noticed:

    • You need to wrap the main content of the page into the <main> tag. It will solve all these landmark issues and improve accessibility.
    • Also footer links are not clickable 😬

    Keep it up! And good luck 🍀

    Marked as helpful