Skip to content
OverviewSolutions
28
Comments
30
Victor
@CodeVee

All comments

  • Victor•1,035
    @CodeVeePosted over 2 years ago

    @faruking Good job. Few pointers

    • Clicking the sun and moon icons break the applications.

    • The App is not responsive on Tablet and Mobile.

    1
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    Hi Pavlo. Fantastic job on this task. I also use rem for my responsiveness. I think it's good and less confusing than em. Few Suggestions

    • Wrap your code with a main tag to fix the landmark accessibility issue.

    • Consider using a different element than the ul or use border-top and border-bottom for your li with padding rather than hr.

    I hope you find this helpful

    Marked as helpful
    1
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    Hi THIH_NEZZY, Great job on this task. Few Suggestions

    • Wrap your code with a main tag to remove the accessibility errors
    <main> 
   <div class="card">
       ...
   </div>
</main>
    • Remove the images from the span in the price-time div and place them side by side with the span
    <div class="price-time">
    <div class="price">
        <img src="./images/icon-ethereum.svg" alt="eth" />
        <span>0.041 ETH</span>
    </div>
    <div class="time">
        <img src="./images/icon-clock.svg" alt="clock" />
        <span>3 days left</span>
    </div>
</div>

    Also update the CSS to look like

    .price-time {
    display: flex;
    justify-content: space-between;
    margin: 7% auto 0 7%;
}

.price,time {
    display: flex;
    align-items: center;
}

.price,time > img {
    margin-right: 5px;
}

.price>span {
    color: var(--cyan);
}

.time>span {
    color: var(--soft-blue);
}

    • I would recommend you research the use of padding and apply it to the parent card rather than using margin on child elements.

    • To overlay properly would then be easier with absolute positioning and flexbox.

    I hope you find this useful

    1
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    Hi Ogunola, Great Job completing this task. Few Suggestions:

    • Wrap your code with the main tag like <main> your code here </main>

    • Set the border and outline to none to remove the white and additional colors from your buttons

    button {
    border: none;
    outline: none;
}
    • For Screens below 375px, I would suggest you don't worry about it.

    I hope you find this helpful

    Marked as helpful
    0
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    @Kehinde13 Hi Kehinde Balogun, Great job completing this assignment. Few Suggestions :

    • Wrap your code with a main tag.

    • Replace your h4 with a h1.

    • Try and center your button

    1
  • Dean Hudek•290
    @deksa89Submitted over 2 years ago

    It's my second challenge and any feedback is welcome. I had difficult time to implement blue color on image on hover and it doesn't work as it is supposed to. I assume that there is way easier solution. Cheers!

    NFT Card

    2
    Victor•1,035
    @CodeVeePosted over 2 years ago

    @deksa89 Hi Dean Hudek, Great Job completing this assignment. Few Suggestions

    • Change the price heading to h2 rather than h3 to clear the accessibility issue.

    • For the hover state. Remove the blue image and set the background of .inner-photo to cyan 00FFF7. Use flex to center both horizontally and vertically the eye SVG.

    0
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    @keltech18 Hi Kelvin Ginikanna, Great job completing the assignment. Few Suggestions

    • Replace the div with class main-box with a main tag.

    • Replace p tag first-paragraph with h1

    • Add a title tag between the head tags. Just after meta should be ok.

    0
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    @Khaltech99 Hi khaltech99, Great job on this assignment, and the cool animation was a nice touch. Few Suggestions

    • Your container seems too wide. Reduce the width and padding as it seems there is too much space on the right.

    • An active class should be applied to your rating to know the selected option.

    • Use const for the other selectors as they are not reassigned.

    0
  • Saleh•390
    @SalehAbuhusseinSubmitted over 2 years ago

    it's my first time Doing Challenge with Jquery nothing fancy I just fetched the Data, I think in vanilla JS it's done with fetch if I am not wrong. any feedback is appreciated.

    HTML SCSS Jquery Ajax

    1
    Victor•1,035
    @CodeVeePosted over 2 years ago

    @Honko-o Hi Saleh, Great job on this. A few things I would say are

    • Good use of destructuring. Adds Clarity

    • JQuery was a bit of overkill. Still good though.

    • p can be an h1 tag for accessibility.

    • a should be a button. Makes for better Markup

    0
  • hakeem•30
    @ladkingSubmitted over 2 years ago

    While I was coding the javascript, I had an issue with the rating buttons, I tried making sure that when one rating button was clicked, the others would go back to default. but I was not able to figure out how to do it. Another issue i faced was that my rate button can be selected in no particular other and the submit button works regardless if the user had selected a rating button or not.

    I'll really appreciate your feedbacks

    Interactive Rating Component using HTML, CSS Grid, Flexbox, JavaScript

    1
    Victor•1,035
    @CodeVeePosted over 2 years ago

    @ladking, Hi Hakeem. It seems you have done this on a private repository. Make it public so that assistance can be offered to you.

    0
  • Victor•1,035
    @CodeVeePosted over 2 years ago

    @Chiku100 Hi Abhilash, Great job on this assignment. I have a few suggestions

    • Wrap your card with a main tag. I would suggest swapping the div tag with attribution class for it.
    • for the attribution styling I'd say change it for it to cover the entire screen like
     .attribution {
    display: flex;
    height: 100vh;
    background-color: hsl(212, 45%, 89%);
    justify-content: center;
    align-items: center;
}
    • Use h1 instead of h2
    Marked as helpful
    0
  • Iván De León Lino•600
    @Lino-OTMSubmitted over 2 years ago

    This was my first time using vanilla JS on my own, I knew what I had to do but before I had to search for methods for my functions. There are some things I could improve such as blocking the submit button until an option has been chosen or maybe another button to come back to the first rating card...

    Any feedback is VERY welcome! :)

    Interactive rating component [Vanilla JS/Sass/BEM]

    1
    Victor•1,035
    @CodeVeePosted over 2 years ago

    @Lino-OTM Hi Iván De León Lino, Great job on this. My only suggestion would be to use const rather than let for your query selectors as the are not reassigned.

    Marked as helpful
    1