Skip to content
OverviewSolutions
33
Comments
25
nmorajda
@nmorajda

All comments

  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    1. Use correct indentation in the HTML code

    2. Reset default CSS styles for browser (now you have default margin / padding).

    3. Change

    .container {
  height: 100%;
}

    on

    .container {
  min-height: 100vh;
}

    and remove the padding, and you will have the element centered vertically as well.

    Marked as helpful
    0
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    Links don't work, I guess that's why:

    function getData(callback) {
   const API = "http://localhost:3000/tracking";
    ...

    and correct the errors visible in the report at the top.

    Happy codding :)

    0
  • nmorajda•945
    @nmorajdaPosted about 3 years ago
                        if(clickIndex === "weekly") {
                         timeFramesCurrent = timeframes.weekly.current;
                         timeFremesPre = timeframes.weekly.previous;
                    }
                         
                    else if (clickIndex === "daily") {
                         timeFramesCurrent = timeframes.daily.current;
                         timeFremesPre = timeframes.daily.previous;
                    }
                    else if (clickIndex === "monthly") {
                         timeFramesCurrent = timeframes.monthly.current;
                         timeFremesPre = timeframes.monthly.previous;
                    }

    can probably be written shorter and just as legible:

    const timeFramesCurrent = timeframes[clickIndex].current;
const timeFremesPre = timeframes[clickIndex].previous;

    or maybe:

    const {current, previous} = timeframes[clickIndex]

    in the second case you have to rename the variables later in the code or

    const {current: timeFramesCurrent, previous: timeFremesPre} = timeframes[clickIndex]
    Marked as helpful
    0
  • David Kalany•50
    @AmazingCukrSubmitted about 3 years ago

    Hi all,

    this is my 2nd attempt to make this card. I would appreciate any help/criticism to make my code better.

    Thanks for all comments!

    Order summary component

    1
    nmorajda•945
    @nmorajdaPosted about 3 years ago

    Look fine on the desktop, but there are some small bugs in the code:

    <div class="Header"> ...

    Don't use uppercase letters at the beginning of the class name, id, etc.

    <div class="text"> ...

    This div element does nothing.

    Why not:

    <p class="text"> ....

    ?

    It is also not responsive and from a width of 400px a horizontal scroll bar starts to appear.

    You also have three errors in the report above.

    0
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    It looks good ... but only on the desktop.

    It is not responsive.

    Use a media query for this purpose.

    Do not specify the height in px (usually the height of the element is to result from its content or the height of the parent element).

    If you use the section element, there should be some header inside. In this case, the IMHO section element is replaceable with a div element.

    Marked as helpful
    1
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    The height of an element should rather result from its content, and not be fixed:

    height: 80vh;

    Use the min-height property if you must, but you don't have to.

    Marked as helpful
    0
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    It looks good but ...;)

    1. For some resolution range, there is no space left and right on the card.

    2. Inside div.stats you have three div elements each with a different class: .companies, .templates., .queries Since their content looks the same and presents related content, it would probably be better to use one of the same name, e.g. .stat

    So that in the future, on a different subpage, you can create such an element .stat in a different place or in a different layout.

    0
  • Dharmik_487•1,740
    @Dharmik48Submitted about 3 years ago

    Another challenge down✌️. This was a lot of fun and taught me a lot's of cool new stuff! I am really proud of the outcome and it motivates💪 me a lot! Thank you Frontend Mentor for this awesome challenge🤗.

    At last if possible, a feedback never hurts. Thank you.

    The Calculator

    2
    nmorajda•945
    @nmorajdaPosted about 3 years ago

    Why is the color information in the JS file? I guess it should be part of a CSS file and JS only handle event handling.

    0
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    Good, but IMHO too many div elements:

    Example 1:

    <div class="info">
  <h1>Order Summary</h1>
 <div class="paragraph">
    <p>You can now listen to millions of songs, audiobooks, and podcasts on any device anywhere you like!</p>
</div>

    What's this div.paragraph for?

    Example 2:

    <div class="payment">
  <ul>
    ...

    Why not right away:

    <ul class="payment">
  ...

    There is also a problem with responsiveness.

    However, these are errors / problems that you will definitely deal with.

    Happy coding.

    Marked as helpful
    1
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    Cześć :) Wygląda dobrze ale kod JS jest moim zdaniem trochę do przerobienia.

    Chodzi mi o to, że zmian wyglądu (theme) powinna być opisana w pliku CSS, a JS tylko obsługiwać zdarzenie i np. dopisywać nazwę klasy w elemencie body. Cała resztę powinien być w pliku CSS.

    Na przykład:

    
::root {
  --body-bg: #e5e5e5;
}

.dark {
  --body-bg: #212121;
}

body {
  background: var(--body-bg);
}

    a cały js to coś takiego:

    
btn.addEventListener('click', () => {
  document.body.classList.toggle('dark')
})

    Pozdrawiam, N

    Marked as helpful
    0
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    Looks fine but keyboard navigation (Tab, Shift + Tab) is not possible because the navigation menu is a bulleted list. Apply buttons or links and the elements will become available.

    Marked as helpful
    1
  • nmorajda•945
    @nmorajdaPosted about 3 years ago

    It looks and works great! Congratulations.

    0