Skip to content
OverviewSolutions
10
Comments
31
Deepak Parmar
@deepak-parmar

All comments

  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @zaidansari42, for best practice I suggest...

    • You should use figure/figcaption tag for the image and the paragraph, caption tag is specifically used for table.
    <figure>
      image
      h1
      <figcaption>
           p
      </figcaption>
</figure>

    This would improve accessibility and won't change the design. Hope this helps 👍

    Marked as helpful
    0
  • Kohsey•40
    @KohseyPowerSubmitted over 2 years ago

    Hi everyone !

    This is my final submition for this challenge. Don't bother to tell me If I made mistakes.

    Feedback are welcome ! Thanks in advance

    Achieved I think

    2
    Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @KohseyPower,

    You can apply multiple classes on an element by separating them with space like this, <div class="column Luxury"></div>, but you cannot add more than one class= attributes to an element like this <div class="column" class="Luxury"></div>

    Marked as helpful
    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @glg-dev, Nice work on the challenge, your design has no flaw at all.

    You've used radio input for rating buttons, which I find the best way to implement this component; so kudos to that. 👏

    • Your page reloads right after clicking the submit button and .thanks section is briefly visible before the page reloads. For that I suggest you use submit event-listener instead of click and prevent submission of the form by using preventDefault( ) method of event object and them call the submitForm( ) function
    submitButton.addEventListener("submit", (event) => {
    event.preventDefault();
    submitForm()
}
    • Next, for accessibility of the component wrap .container and .thanks elements using main tag and .attribution element using footer

    Hope these helps, keep coding! 👍

    Marked as helpful
    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @Arslanj9,

    • Your path to main.js is not working, remove / from the beginning of it. It should be just js/main.js or add . in front on it for best practice like this, ./js/main.js

    • Submitting a form reloads the page, so after clicking the submit button the page reloads and returns to its original states; to prevent that use submit event-listener

      submitBtn.addEventListener("submit", (event) => {
    event.preventDefault()
    btnClick()
  })

    event.preventDefault() will stop the submission process of the form.

    • For accessibility issue, wrap your header using main tag

    I hope this work out. keep coding 👍

    Marked as helpful
    2
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @anhhuynh1506, Visually your work looks so great.

    You just have resolve some accessibility issues...

    • Document should have one main landmark: for this wrap your whole section element using main tag.
    • Page should contain a level-one heading: heading elements (h1-h6) should always be in order and should always start from the level-one heading h1. For your "Order Summary" heading use h1 instead of h2. You already have font-size property set for the heading, so you'll just have to make a few changes there.

    The design won't change a bit, but these changes will improve the accessibility of your component. Again nice work on the challenge. Keep coding! 👍

    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @lauriselvijs, Your work looks amazing, nice work on theme switching!

    • The dice button and the divider becomes hard to spot in light mode. Applying filter: invert(1) to both will work just nicely.
    • and also add cursor: pointer to dice button and theme-switching button for better UI feedback.
    Marked as helpful
    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @kaminskimaciej, visually your works looks great!

    First of all, you need to renew the screenshot and regenerate the report of your submission.

    After then, you just have to eliminate some of the accessibility issues...

    • Heading elements should always be used in order from h1-h6. For your .title element use h1 tag instead of h2, it won't change anything in the design.
    • The same error is also caused by the h5 tag later in your code. I suggest not using h5 and use <p> tag for it, since it is more like a label than a header.
    • Next, you need to wrap your whole .container element inside the landmark tag <main>, cause all pages must have a landmark element.

    You code looks decently clean, I suggest using some code formatter to automate that part.

    I hope this clears up the issues. 👍

    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @KAMA-Kasckak-Martin,

    your image is not being loaded, replace / with ./ at the beginning of your image's path (like this, ./images/image-qr-code.png)

    • / at the beginning means it'll looks for files at the root of the URL which is https://kama-kasckak-martin.github.io but your solution is available at https://kama-kasckak-martin.github.io/QR/

    • ./ will look for files from where it is currently hosted at.

    If this is confusing I recommend this article from MDN Docs https://developer.mozilla.org/en-US/docs/Learn/Common_questions/What_is_a_URL Especially, Absolute URLs vs relative URLs section.

    and this one from W3Schools

    https://www.w3schools.com/html/html_filepaths.asp

    These might clear out lots of thing Pathing doubts.

    Marked as helpful
    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @pouyanasr, visually your work looks great, nice work!

    but your use of h4 and h5 right after using h1 is raising an Accessibility Issue: Heading levels should only increase by one. There cannot be discontinuity between heading elements, these should be used in order. h2 should follow after h1 and so on.

    I suggest that you use p tag stats and just give them font-size value just as h5 and h4 which you can see in the browser console. This way the design won't change and will be still accessible.

    Other than that I see that your code has inconsistent spacing, It's a bit of a hassle to make the code look clean, I recommend using code formatter in your code editor.

    I hope these helps, keep coding! 👍

    Marked as helpful
    0
  • Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @DylanMuldoon, instead of div to hold .car-card element use the semantic tag <section>. That would improve the accessibility further more since articles have sections.

    1
  • Vasyl•550
    @vasyaqweSubmitted over 2 years ago

    It was a pretty ease one. Didn't have any difficulties along the way. One thing I think would be cool to implement is the typewriter animation when the advice comes in. I googled it a little but no results.

    Advice generator

    1
    Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @vasyaqwe,

    For typewriter animation you can use JavaScript's setInterval() function to add each letter to .advice-text paragraph once advice is fetched. https://developer.mozilla.org/en-US/docs/Web/API/setInterval

    Still, this would take some research. I'm sure you'll have fun.

    And for the accessibility issue, I suggest use h1 for the .advice-number element instead of span, because screen reader devices won't know where to start reading the content without a landmark element such as h1. You've already set font-size for .advice-number so using h1 won't affect the design but will improve accessibility of your app.

    Hope these helps. 👍

    Marked as helpful
    1
  • Amina Ibrahim•10
    @aminafolioSubmitted over 2 years ago

    I couldn't get the footer to go to center and I haven't learnt css grid yet. I will start learning it today. Other than the concepts I dont know, is the project okay, it terms of responsiveness and closeness to design?

    QR Code Landing page

    1
    Deepak Parmar•490
    @deepak-parmarPosted over 2 years ago

    @aminafolio,

    The attribution section isn't necessary to keep since it's not part of the designs and might interfere with the design. I usually don't keep attribution.

    However you need to take care of some accessibility issues. Screen reader devices requires landmark element to jump on when page loads therefore...

    • Wrap your .root element with landmark tag main
    • prefer to wrap .attribution using footer tag, if you're planning to keep it.

    Other than these, your work looks nice and code is decently structured. Nice work 👍

    Marked as helpful
    0