Skip to content
OverviewSolutions
22
Comments
8
Abreham Nigussie
@DreamCoder7

All comments

  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Hey, Nkiruka Ebele👋

    Great effort on this challenge!👏

    A few things I'd like to suggest are,

    • Displaying the images from your html using img tag isn't problematic but you can take the advantage of background-image: url('..') to switch between the mobile version of your img to desktop version img. This way you can a void the repetition.
    • It's more interesting if you add a transition properties on the anchor tag <a></a> in your transform section
    • I also notice that the modal windows which is toggle or fire by clicking the hamburger menu it's not set to a high z-index value, In <section class="design-photography"></section> it overlap by the text. So you can quickly fix that and also adding a backdrop can help the modal to stand out from the page not relevant but also good practice.

    Hope you find this helpful!😊

    Happy coding😎

    0
  • BreinerJT•530
    @BreinerJTSubmitted over 2 years ago

    This challenge took me a while because I don't know much about async / await functions, I don't feel good about my knowledge in this kind of functions so if you know where can I learn or read more about this I would really appreciate it if you share it with me. Any feedback is welcome!

    Expenses chart component

    2
    Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Hey, BreinerJT👋

    Nice effort on this challenge!👏

    Overall, your solution looks great! One small thing I'd like suggest is that instead of outputting the chart data one by one using html like you did, you can dynamically output from JavaScript. Also you can add a Gaurd clause incase if the user failed to load, More about Gaurd clause

    async function getData(url){
    let fetchData = fetch(url)
    let response = await fetchData;
// Adding Gaurd clause 
    if (!response.ok) return;
    let data = await response.json()
    return data;
}

 async function getCharts(){
    const chart = document.querySelector(".charts");
    const data = await getData('data.json');

    let html = null;
    data.forEach(e => {
       html =  `
        <div class="chart-container">
            <p class="day" data-id="mon">${e.day}</p>
            <div class="js-chart chart" id="mon" style="height: ${e.amount}%"></div>
            <label class="js-label" for="mon">${e.amount}</label>
        </div>
        `
    chart.insertAdjacentHTML("afterbegin", html);
    });

// Not needed
/*
    const amounts = data.map(e => e.amount)
    let maxValue = Math.max(...amounts)
    let heights = amounts.map(e => e / maxValue * 100)
    charts.forEach((value, index) => {
        value.style.height = `${heights[index]}%`
    })
    charts[day].classList.add("current-day")
    labels.forEach((value, index) => {
        value.textContent = `$${amounts[index]}`
     })
*/
}

getCharts()

    Hope that helps!🤗

    Happy coding.😎

    Marked as helpful
    0
  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Hi, system-fresh👋

    Good effort on this challenge!👌

    Here are a few things I'd like to suggest,

    • Perhaps using a little bit of semantic HTML help you with accessibility and much easier to read. like wrapping the hole code into <section class="productbig">....</section> because it's more sense to have it's own section. And also wrapping the testimonial part like this is also practical
    <figure class="box">
        <div class="profile"></div> 
        <blockquote>
                   <p>...</p>
        </blockquote>
</figure>

    Read more semantic HTML

    • Instead of using the <img src=".." alt=".."/> to display the icon-star.svg you can use the css background-image to avoid the repetition you made on HTML.
    background-image: url('..');
background-size: 20px 20px;
background-repeat: repeat-x;
background-repeat: space;
....
    • You already set the padding: 0; and 'margin: 0;' using the universal selector (*) to reset all the element means any style that you put in the body gets inherited you don't need to repeat into the body{} tag

    Hope you find this helpful.😊

    Keep coding!😎

    0
  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Hello, Tahmeed!👋

    Nice work on this challenge!👏

    Here are a couple of things I'd like to suggest,

    • Using inline style for both style style.css and script script.js aren't recommended way, the reason is because of semantic markup, maintainability, reusability, and scalability. Specially on large project. check out this article it may help
    • Instead of element selector like .top-left-head ul li using class or id is more practical way

    Hope that help!😊

    Happy coding!😎

    1
  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Great job, Ugiomoh Daniel

    But there is a little bit of a problem with your semantics usage. you use a div element whenever you want to use a wrapper or container. The problem with this is it's difficult for screen readers. You can fix or prevent this kind of problem using WAI-ARIA Roles. ARIA roles provide semantic meaning to content, allowing screen readers and other tools to present and support interaction with object in a way that is consistent with user expectations of that type of object. For more

    And I also notice that you use two heading element twice and that isn't a good practice. Here are some guidance that should help:

    /*
This is the two heading that use the same heading element and it's not mandatory but instead of using h1 in this case you can use h4 or h5 because h1 indicates the most important (or highest-level) heading on the page.
*/
/*
<h1>How did we do?</h1>
<h1 class="ht">Thank you!</h1>
*/

/* Instead of using onclick="" you can select the parent element and by using Event propagation you can do the same thing but applying DRY principle.*/
/*
<div class="numbers" id="numbers">
        <button onclick="star1()" id="A" class="botn">1</button>
        <button onclick="star2()" id="B" class="botn">2</button>
        <button onclick="star3()" id="C" class="botn">3</button>
        <button onclick="star4()" id="D" class="botn">4</button>
        <button onclick="star5()" id="E" class="botn">5</button>
 </div>
*/
const parentBtnEl = document.querySelector('.numbers');

parentBtnEl.addEventlistener('click', function(e) {
     const btn = e.target;

   if (btn.closest('A')) {
   // Do something....
   }
......
});
    0
  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Great job, Letícia dos Santos,

    But there is a big problem with semantics. I noticed that all the images don't have alt attribute that is one of the reason you have an ACCESSIBILITY issue and also you should only use id for unique purposes instead you can use class.

    Here are some guidance that should help:

    /*
id: is unique in the context of a web page; It can not be duplicated.
class: can appear multiple times on multiple element.
[For more explanation: ] (https://css-tricks.com/the-difference-between-id-and-class/)
[More: ] (https://www.w3schools.com/html/html_id.asp)
*/

/*
use alt attribute:
<img src="./images/icon-ethereum.svg"  alt="Ethereum Icon"/>
*/

/*
// Basic setup using universal selector feel free to include at the top of your CSS code 
*,
*::before,
*::after {
      margin: 0;
      padding: 0;
      box-sizing: border-box;
}

The Universal Selector  (*):  Literally the asterisk character. It is essentially a type selector that matches any type. Type meaning an HTML tag like <div>, <body>, <button>, or literally any of the others.
*/
    1
  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    great job, Willians;

    But I see some issue with your styling and structuring your HTML file. First separating your style from HTML or ignoring inline style is way better for reading your code. Second you have a lot of accessibility issue because of your HTML structure.

    Here are some changes I made in browser that should help with accessibility issue:

    /*
<a href="">
  <img class="img" src="/images/image-equilibrium.jpg" alt="" />
</a>
*/
// The alt attribute is a semantic html (means a certain element have actually meaning and purpose attached to them)so don't ignore it. it's use full for accessibility and also SEO 
<a href="#">
  <img class="img" src="/images/image-equilibrium.jpg" alt="some image" />
</a>

<div class="lado">
   // why bothering using (<h4></h4>) and also even if you want to use you should use (<h3></h3>) tag because Heading levels are important!
   /*<h4><img class="img1" src="/images/icon-ethereum.svg" alt="">0.041ETH</h4>*/
  // you can fix like this:
  <img class="img1" src="/images/icon-ethereum.svg" alt="ethereum Icon">
  <p>0.041ETH</p>
</div>

 /* hr {
        background-color: hsl(215, 32%, 27%);
        border-color: hsl(215, 32%, 27%);
        height: 1px;
        width: 90%;
      } */

      // instead of using <hr /> tag you can do it like this. but still if you prefer using <hr /> tag you can use but it better to use like this
      .lado2 {
        border-top: 2px solid hsl(215, 32%, 27%);
      }

      /* Why?? 
      img.img2 {
           vertical-align: middle;
      }*/

    // Instead do this
   .im2 {
       vertical-align: middle;
    }
    Marked as helpful
    0
  • Abreham Nigussie•470
    @DreamCoder7Posted over 2 years ago

    Well done, great job.

    But there is problem with your styling. First instead of using inline style you can use external file and link to your HTML that way it's better to view your code. Second I kind of looking your styling code and I found that some of your code not necessary or duplicated and also messing some style.

    Here are some changes I made in browser that should help with remove unnecessary code:

    /*You should name your HTML tag as necessary for the feature*/
div{
  text-align: center;
}

/*This is the parent element*/
main {
    display: flex;
    justify-content: center;
}

.card {
    background-color: white;
    /* Not necessary you can center your card by styling the parent element */
    /* display: flex;*/
    /*justify-content: center; */
    /* flex-direction: column; */
    /* margin-left: 500px; */
    /* margin-right: 500px; */
    /* padding-top: 15px;*/
    /*padding-left: 5px;*/
    /*padding-right: 5px; */
    padding: 15px 5px 0 5px; // You can do it in one line
    border-radius: 10px;
    box-shadow: 0 4px 8px 0 rgba(0, 0, 0, 0.2);
    transition: 0.3s;
}  
```;
    Marked as helpful
    0