Skip to content
OverviewSolutions
16
Comments
37
enochlee
@iamenochlee

All comments

  • enochlee•600
    @iamenochlee

    This challenge does not really define any hierarchy for semantic text, so you do not need to worry about what heading tag you should use, congrats on your solution, keep coding!

  • enochlee•600
    @iamenochlee

    1)add to the body { width: 100vw; overflow-x: hidden} 2)There are several ways to toggle the navbar with knowledge of javascript. You can go all in using javascript just like this, make sure the navbar has a display of none in the css limited to mobile media query,

    //grabing the elements, 
const toggler = document.querySelector(".menu-toggler");
const navBar = document.querySelector(".list--container");
const body = document.body;
const icon = toggler.querySelector("img");

//opening navbar and closing navbar

/*creating custom function to handle the click*/
const handleOpen = (el) => {
  el.style.display = "block";
  icon.src = "./assets/images/icon-menu-close.svg";
  icon.alt = "Close Navigation Menu";
  body.style.overflow = "hidden";
};

const handleClose = (el) => {
  el.style.display = "none";
  icon.alt = "Open Navigation menu";
  body.style.overflowY= "scroll";
  icon.src = "./assets/images/icon-menu.svg";
};



toggler.addEventListener("click", () => {
  if (navBar.style.display === "block") {
    handleClose(navBar);
  } else if (navBar.style.display ==="none"){
    handleOpen(navBar);
  }
});

/*closing the navbar on click outside*/
navBar.tabIndex = 0
document.addEventListener("click", () => {
  if (
    navBar !== document.activeElement &&
    body.getBoundingClientRect().width < 720
  ) {
    if (document.activeElement !== toggler) {
      handleClose(navBar);
    }
  }
});

    if you prefer a css approach you will have to add and remove a class of "open" to it when the button is clicked, the use that class to handle the display in the css.

    Keep Coding,

    EDIT: cleaned up the function

    EDIT2: While that works it has a bug, you have to click twice to open the navbar at first , here is a new solution

    
//custom functions
function handleClose(el) {
  el.classList.remove("open");
}

function handleIcon() {
  if (toggler.classList.contains("open")) {
    icon.src = "./public/assets/images/icon-menu-close.svg";
    icon.alt = "Close Navigation Menu";
  } else if (!toggler.classList.contains("open")) {
    icon.src = "./public/assets/images/icon-menu.svg";
    icon.alt = "Open Navigation Menu";
  }
}

//toggling navBar
toggler.addEventListener("click", () => {
  toggler.classList.toggle("open");
  navEl.classList.toggle("open");
  handleIcon();
});

//closing on click outside
navEl.tabIndex = 0;
document.body.addEventListener("click", () => {
  if (document.activeElement !== toggler) {
    if (
      navEl !== document.activeElement &&
      document.body.getBoundingClientRect().width < 720
    ) {
      toggler.classList.remove("open");
      handleClose(navEl);
      handleIcon();
    }
  }
});

css

.open{
display: block}
    Marked as helpful
  • enochlee•600
    @iamenochlee

    Your media-query kicks in late, leaving your solution to stretch all the way to 1500px, reduce this to 900px, also add this style.

    @media screen (min-width: 500px) and (max-width: 900px) {
.container {
padding: 10em;
}
button {
margin-bottom: 1em;
}

    I see you made use of semantic elements and relative unit, good job, you should also start approaching layout from a mobile-first approach. Keep Coding

    Marked as helpful
  • enochlee•600
    @iamenochlee

    Hey there, congratulations on your Challenge, Your solution seems to have a vertical scrollbars, well you can correct this, by using this styles

    body{
margin: 0;
width: 100%;
overflow-x: hidden;
}

header{
width: 75%;
}

.pattern{
left: 69em
}

    But this is not an efficient way, a much better way is to add the pattern as a background-image to the body for just tablet to desktop screen-sizes, since its not needed on mobile, or perhaps reposition outside the viewport for mobile, then position it top, right, i will link an article on working with two or more background image, also an article on css resets. Keep coding!.

    background-images css resets

  • enochlee•600
    @iamenochlee

    I see you are using grids, your layout breaks between 550px and 900px, ideally a tablet screen, first i think you should redefined the grid-template-column at this particular sizes to repeat(2, 1fr), since you are using tailwind that should be yourmediaquery:grid-col-2`, you can then reste it back to three at large screen sizes, while from 900px to 1200px, increase the width of the container, so the grid has more space. You can create custom media query in tailwind, check out the docs. Do well to check your website on responsive mode in your browser. i hope this helps, Happy coding, Great work!.

    Marked as helpful
  • enochlee•600
    @iamenochlee

    Looks good!, i noticed some few stuff, first i think you should disable the button or give a notification while the link is being shortened, makes for awareness, closing the mobile nav on click outside, also at 1600px screen width your block-padding for the page is very little, you might want to keep the same padding it had at 1440px. Happy Coding!

    Marked as helpful
  • enochlee•600
    @iamenochlee

    Not familiar with using github pages for react deployment, but , You can use vercel, just connect you git account and you are good to go. Well ofcourse make sure you build the project. If i couldn't convince you, well here is an article on your question Github Pages. I believe you will have to add github pages as a package dependency

    Marked as helpful
  • enochlee•600
    @iamenochlee

    Hey there, there's certainly no no one way of solving, i would have taken this same approach on this challenge, great job, keep coding.

  • enochlee•600
    @iamenochlee

    Nice, congratulations on your matching solution. It's standard practice to make use of mobile first approach. As for grid or flexbox it depends on the layout you are trying to achieve, there are no rules or conditions to use them, just use what will deliver the best results. Keep Coding

  • enochlee•600
    @iamenochlee

    You did well, your website looks great on mobile, the html errors are wrong practice you made use of, its nice to go through them and make necessary changes, your screenshot has a whitespace because your solution doesn't have the same height with the exercise it's not a big deal, since you don't have access to the design files. Keep Coding.

    Marked as helpful
  • enochlee•600
    @iamenochlee

    you did a great job here, we all face such difficulties at first, as you engage in more exercise, it all be bygone, i notice some few stuffs;

    1. your image isn't quite fitting in the container, this is due to the padding in the article text, just a quick fix, apply
    .image{
  height: 100%;
  background-size: cover
}

note its not a good practice to apply both height and width on images, but since this is 100%, you can get away with it.





2. You don't have to apply border-radius to the image also, remove the border-radius from the image, apply it only to the article container.



.article{
  border-radius: 10px;
  overflow: hidden;
}

overflow hidden will enforce the border-radius on all edges


congratulations on your first solution here, go through the solution reports and fix the issues.
keep coding mate.
    Marked as helpful
  • enochlee•600
    @iamenochlee

    Hey there, congratulations on your solution, its actually an onclick even for desktop also, you should also make sure when outside of the dropdown is clicked it closes, you might also want to align the dropdown icon and the text properly, also from 500px to 750px, your layout breaks, the image expecially, you can use a media query to fix this, i'd advice you start making use of clamp for fontsizes, it makes for responsive text.

  • enochlee•600
    @iamenochlee

    Great job done here, this actually works, i have seen several solutions on this that doesn't actually shorten the url, congrats on your solution. You can go through the reports and fix the issues

  • enochlee•600
    @iamenochlee

    first challenge here! congrats. here are my observations. introduced the media query later.(min-width: 60em, seems fair) reduce the font-size of the text, this way your button will be able to show up, set .container { overflow: hidden } this way you can work with the space and apply adequate margins and font-sizes. For the button, use a button tag '<button type="button">...Buy Now</button>then apply width and height, you can also use an <a> tag to achieve it with paddings, you will have to apply adisplay: inline-block` if you use an <a> tag for the buy now, i will suggest you use a button tag.

    great job done with the image responsiveness. Also you should start using responsive units like rem and ems instead of px, read more here Rem an Em

    Marked as helpful
  • enochlee•600
    @iamenochlee

    hey there, this is nice, I noticed some few stuff, the timer and clock are not well aligned, just put them both in a container and apply a display: flex; justify-content: space-between; align-items-center, You seem to be using some weird property for margins which results in your container not properly centered, try to fix this, keep coding.

    Marked as helpful
  • enochlee•600
    @iamenochlee

    Great job poala, this is nice!

    Once you use an img tag to import an svg, the fill property no longer works it's now considered and image, you can get around this by using the svg directly, then the fill property will work, you can also use an object tag to import it.

    <object data="your.svg" type="image/svg+xml">
</object>

    OR

    <svg>
<use href="filename.svg"></use>
</svg>
  • enochlee•600
    @iamenochlee

    congrats on your solution, you can still apply some padding and margin changes to it, the image looks streched, this is due to the fact that you applied a height to it, avoid appling heights to images, use the width property only, you can always update your solution once you get your javascript going. I hope this helped you out. Keep coding;

    Marked as helpful
  • enochlee•600
    @iamenochlee

    Hi there, your design is responsive, congrats 👏 But your container is not well centered, while there are many ways to do that I find using display:grid; place-items: center; to the parent element containing the container very easy, I hope this helps you.

    Keep Coding. Congrats on your solution

    Marked as helpful