Skip to content
OverviewSolutions
19
Comments
22
David Osorio
@osoriodev

All comments

  • David Osorio•560
    @osoriodev

    Hi Ali

    There's a problem. When you deploy a project with GitHub Pages, the root directory(/) refers to username.github.io, not the repository itself. You must use a relative path, in this case it must be body { background-image: url("../images/bg-desktop.svg"); }

    Btw, remember to use the aria-label attribute for <a> tags when there is no discernible text.

    I hope it helps.

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello @JoshuaAD

    The best thing you could do is work with your own styles. It's true that Figma provides certain styles, but they are not actually optimized to work on a real website. You can use Figma to check for more specific things, for example: font sizes, colors, paddings, widths, etc.

    Btw, you have some accessibility issues. There must be an h1 on the page, it's very important for semantic and accessibility reasons.

  • David Osorio•560
    @osoriodev

    Hello @titocs, you have some accessibility issues.

    • You are using an h1 for each card title. There can only be one h1 on the page.
    • You have an h3 for the person's name and an h5 for the text "Verified Graduate", that doesn't make sense.

    When you use HTML headings, you must follow a logical order like a book's index.

    • h1
      • h2
        • h3
        • h3
        • h3
      • h2
      • h2

    Headings are usually separated by an article, a section, etc. I hope it helps you 👍

  • David Osorio•560
    @osoriodev

    Hello. I see your styles don't work. <link rel="stylesheet" href="/styles.css">

    You must add a dot to the href: <link rel="stylesheet" href="./styles.css">. This is to indicate that the file is on the same path, the same for the images.

    Btw, remember that there must be an h1 on the page.

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello Obuhri.

    Your solution looks great. As a suggestion you can use a tags for the social media icons, I think it is more appropriate than using a span. I hope this helps ✌

  • David Osorio•560
    @osoriodev

    Hello there.

    I suggest you use a button element for the share option and put the icon inside, same for social media icons, it is better to use a tags and put the icons inside. This for better accessibility and semantic HTML.

    Don't forget to use the aria-label attribute as there is no discernible text.

  • David Osorio•560
    @osoriodev

    Hello @Saran-73

    I was checking the website and I see that you set the main with grid-template-rows: 40% 50%, but you didn't set a specific high, so the percentages don't know what value to refer to. Maybe this is the reason why an bug occurs and this property is ignored.

    In fact, if you remove that property, the website will look the same as the screenshot. I suggest you remove that property and modify the padding of the sections below.

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello 👋

    A couple of things that I can recommend you

    • Try not to use <br> tags, since these modify the document flow and that must be done from the CSS. If you want to create a spacing, you can use the margin properties.
    • Use more semantic HTML, for example Sedans, Suv and Luxury are titles, in this case you can use HTML title tags, the span is for text that has no relevance.

    Overall, your project is a good start. Keep coding 👨‍💻

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello @ClariceAlmeida 👋

    You have some issues with your HTML, for example:

    • You are using an a tag to send email, it must be a button, the a tag is for links only
    • The <div id="text"> must be an h1 tag
    • The text tag is not a valid html tag, you can use other tags instead, for example span

    As for JavaScript, I recommend that you use regular expressions for validation.

    Overall, your design looks great. I hope it helps. 👍

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello @VictorGelado 👋

    To solve the problem, you can do this:

    • Add position: relative to the wrapper, this way the images will be positioned with respect to this element
    • It also changes the attribution styles for these, so it looks good on mobile devices.
    .attribution {
  width: 100%;
  position: absolute;
  left: 0;
  bottom: 4px;
  text-align: center;
  font-size: 16px;
  color: #fff;
  z-index: 4;
}

    I hope I've helped 👍

  • David Osorio•560
    @osoriodev

    Hello @akinsanyajoseph 👋

    Your solution looks good. I see that the eye icon is not centered properly, you can do this to fix it:

    .overlay-icon {
  position: absolute;
  top: 50%;
  left: 50%;
  transform: translate(-50%, -50%);
}

    With this it should work fine.

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello @ArnasLuksas 👋

    I was reviewing your code and I see that you are using a validation with includes(). In this case it would be appropriate to use regex (regular expressions) for validation. In short, a regular expression is a sequence of characters that forms a search pattern. For example, a regex for the email can be:

    const re = /^[a-zA-Z0-9_.-]+@[a-zA-Z0-9-]+\.[a-zA-Z]+$/

    If you want to know if an email is valid, you can use the test() method, this will return true or false.

    re.test("example@company.com") // true
re.test("@example") // false

    If you want to know exactly how characters work, you can find a longer article on the subject here: Regex

    A couple of tips on accessibility

    • Since there is no text in the email button, you must to use the aria-label attribute, for example: aria-label="Send email"
    • Use the appropriate input tag, in this case <input type="email">

    I hope it helps you. 😁

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello @TerrenceArceo 👋

    You can easily add a border with border: 1px solid white, and to make it fit the image add: border-radius: 50%

    I would recommend using a tags for the creator's name and another within the title, since in theory it is a link to the person's profile and the NFT itself.

    Overall, you result looks great 👍

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello 👋

    • You have a <div role="main"> , this is unnecessary because there is already a main tag for semantic HTML.
    • In mobile view(375px), the layout is broken. You are using this to center the card in desktop view, but on mobile it doesn't work:
    div:first-child {
  position: absolute;
  top: 50%;
  left: 50%;
  transform: translate(-50%, -50%);
}

    You can remove those styles in mobile view and do this:

    div:first-child {
  width: 330px;
  border-radius: 8px;
  overflow: hidden;
  margin: 0 auto;
}
    • Finally, I suggest you use classes to style the elements, you can try using the BEM methodology.
  • David Osorio•560
    @osoriodev

    Hello @Zeighnab. Great solution. Some things you might consider are:

    • Have a CSS file with all the styles, independent of the html. This is because if the project grows it can be more scalable.
    • In the mobile view you set the container with display: flex and flex-direction: column, this is unnecessary because the article element has default block display. That makes each element below the other.
    • If you want the card to have a border-radius, you just need to apply it to the card and set a overflow: hidden. This will save you lines of code.
    • And the buttons must also have the Lexend Deca font family.

    Hope this can help you.

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello Vinicius.

    You have an img tag without the src attribute, it is always necessary to set this attribute and the alt attribute, for accessibility.

    And yes, it is possible to do the accordion without JS, but you have to use some tricks with CSS. Using checkbox inputs (or radio inputs if you want only one question to be displayed at a time), the checked pseudo-class and the general sibling operator: ~ . With this you have the basic logic to make the accordion.

  • David Osorio•560
    @osoriodev

    Hello there.

    You have some issues in the HTML

    • For accessibility, all buttons must have text. In case there is only one icon or image, the aria-label attribute must be used (for example: <button aria-label="Share"></button>)
    • The headings should increase by one (You are using a h3 after the h1)
    • It is recommended to put the script before closing the body tag, this is to ensure that the DOM is ready when the script is executed.

    Anyway, your result was great. I hope I have helped.

    Marked as helpful
  • David Osorio•560
    @osoriodev

    Hello. 👋 Some things that could improve are:

    • You are using 2 h1 tags, it is recommended to use only one tag for each page. (If you want to style the two title texts, you can put two span inside h1)
    • Headings should increase steadily. You have an h3 after the h1, the correct thing to do would be to use h2
    • You should use more semantic HTML (for example: article, section, etc.)
    • And you have a typo in the line 8, you have opened the link tag twice.

    Overall, your result was very accurate, congratulations.

    Marked as helpful