Skip to content
OverviewSolutions
10
Comments
13
Dêvid Teófilo
@devid8642

All comments

  • Dêvid Teófilo•240
    @devid8642

    Firstly, congratulations on the solution.

    I have some considerations about your project:

    Regarding the structuring of your page, semantically speaking, all this content could be wrapped in an <article>, which could be inside a <main>. See more about HTML semantic tags here.

    Regarding the style, notice that the design predicted that the "add to cart" button has a specific behavior when the mouse passes over it. I don't know if you know it, but it is possible to implement this using the :hover pseudo-class.

    Again, congratulations on the solution and I hope you found my comment helpful.

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    Firstly, congratulations on the solution.

    I have some considerations about your project:

    I just have to say about HTML, semantically speaking all this content could be wrapped in an <article>, which could be inside a <main>. See more about HTML semantic tags here.

    Again, congratulations on the solution and I hope you found my comment helpful.

  • Dêvid Teófilo•240
    @devid8642

    Firstly, congratulations on the solution.

    I have some considerations about your project:

    Regarding structuring the page with HTML: note that the card in question is self-contained content and therefore semantically it would be inside an <article>. Furthermore, it is important to use titles respecting the hierarchy, remember that color and size issues can be resolved later using CSS.

    Regarding styles, I just have to comment on a more effective way to center the card:

    .main {
   display: flex;
   flex-direction: column;
   align-items: center;
   justify-content: center;
   height: 100vh;
}

    This is enough to align the card horizontally and vertically using flexbox.

    Again, congratulations on the solution and I hope you found my comment helpful.

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    Firstly congratulations on your solution.

    I have some considerations about your project:

    Regarding HTML, I believe it would be interesting to place the content about nutrients in a table instead of a list, from the point of view of HTML semantics this would be more correct. See more about tables in HTML here.

    As far as style is concerned, it is necessary to make some adaptations so that the page works on mobile. With CSS it is possible to do what we call responsive design, which is a design that adapts to different displays. To do this, the main resource we use are Media Queries. I believe that a good initial step to try to apply them to this page would be to reduce the width of the main for smaller displays.

    Try this and see the result, to test the page on a smaller display you can resize your browser window or even look in the browser's developer tools about responsive mode.

    I hope you found my comment helpful and again congratulations on the solution.

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    Firstly congratulations on your solution.

    I have some considerations about your project:

    It is interesting to structure the page content within a <main> tag instead of a section. As this type of card is self-contained content, it is interesting to place it inside an <article>. And finally, the card footer can be placed inside a <div>, since the <footer> tag is used for the page footer and not for the content footer. See more about the semantic elements of HTML here.

    Regarding the design, I believe it is interesting to make just a few adjustments. For example, increasing the size of texts, adding a border-radius in "Learning", changing the font weight to bold for the date and author's name and finally creating a shadow effect without using borders, to do this research on the box-shadow property.

    Again, congratulations on the solution, I hope you found my comment helpful.

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    Firstly, congratulations on the solution.

    I have some suggestions for your project:

    Consider structuring all the content on your HTML page using semantic tags, in this case <main> and <article> for the card. See more about semantic tags here.

    It's not interesting to use tags like <br> to position like you did, consider doing this in CSS because it breaks your layout. As you centered the card using position, depending on the display, the texts may end up leaving the card.

    I recommend that you remove the <br> and to center the card use the following, considering the card inside <main>:

    main { /* or .container */
   display: flex;
   align-items: center;
   justify-content: center;
   min-height: 100vh;
}

    Centering the card without using position and removing <br> will certainly ensure that the layout is more uniform across existing display varieties and facilitate responsive design.

    Even though you applied the sources to the project, you did not upload the assets directory to GitHub and therefore they do not load on the page.

    As this is a simple project, the responsive design probably became a problem due to the layout techniques you chose to use, I recommend using flexbox to center the card and without using <br> to position the texts (research the property text-wrap) you probably won't even need to apply any additional CSS rules in the mobile media query, just change the width of the card.

    I hope you found my comment useful, again congratulations on the solution.

  • Dêvid Teófilo•240
    @devid8642

    Firstly, congratulations on the solution.

    I have some suggestions for your project:

    Consider using flexbox to center the card instead of using large padding and margin values in the main and body. You can remove these values and use them in main:

    main {
   display: flex;
   align-items: center;
   justify-content: center;
   min-height: 100vh;
}

    Also consider using relative units for the values of margin, padding and font-size, in this case REM. I recommend that you research the usefulness of doing this as it is good practice.

    Finally, the formatting of your HTML file is a little inadequate. I recommend using a formatting tool like prettier (https://prettier.io), which makes the job of keeping the file well formatted much easier. This tool integrates with the main code editors that currently exist.

    Again, congratulations on the solution. I hope you found my review helpful.

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    Hello, I'm here again. I was also wondering if there was any way to use color variables in the way you described. Fortunately I found this css resource: https://www.w3.org/TR/css-color-5/#relative-colors. I believe it serves our case well.

    Here is an interesting explanation of this new syntax: https://developer.chrome.com/blog/css-relative-color-syntax/.

    However, as this is a new feature, it is not yet supported by all browsers (currently when I write this comment, the most recent version of Firefox does not support this feature, for example).

  • Dêvid Teófilo•240
    @devid8642

    Firstly congratulations on the solution, I just have a small tip in relation to what has already been said:

    To center the card, in addition to using a flex display as already mentioned, consider also using: height: 100vh; to completely center;

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    First of all, congratulations on the solution, but I have some points to consider:

    Use an <h1> instead of an <h2>. For reasons of semantics and SEO, it is interesting that you first have an <h1> and then an <h2>, even if the style of the <h1> doesn't make sense because you can change the style in CSS;

    I believe it would be interesting to adjust the background color of the page to a lighter blue. In the style-guide.md file you have some useful color references but always feel free to adjust them as necessary;

    Again congratulations on the solution.

  • Dêvid Teófilo•240
    @devid8642

    First of all, congratulations on the solution, but I have some points to consider:

    Use the serif font available in the project in the appropriate titles;

    Use the CSS ::marker pseudo-element to style list markers;

    Change the color of the texts at the beginning of the page so that they look like the texts on the rest of the page;

    Again congratulations on the solution.

    Marked as helpful
  • Dêvid Teófilo•240
    @devid8642

    Hello, firstly congratulations on the solution and continue improving it (I see that you have already centered the card), but I would like to bring a few more points for you to consider:

    Regarding the structuring of the content, that is, the HTML: consider using appropriate titles according to their semantics and not the design they present, as this can be adjusted with CSS. So the first title of the page, for semantic and SEO reasons, could be an <h1> instead of an <h2>. Another important point is the table, consider using <th> instead of <td> for table headers.

    Regarding design and layout, I believe you are still working on it, if you have any questions feel free to comment here.

  • Dêvid Teófilo•240
    @devid8642

    Hello, firstly congratulations on the solution, but I want to share with you some points to consider:

    It would be interesting, from the point of view of good practices and SEO, to structure the page content using the appropriate HTML tags instead of just div. The card title could be <h1>, the card's descriptive text could be <p> and the small message below the card could be understood as the page's <footer>.

    About the design: to improve the design of your page, consider using the font informed in style-guide.md and adjusting the text colors according to this same file. The use of borders on the card and image are not in the design preview, of course you can consider adding them or not according to your preference but I think it doesn't look very good.

    About the layout: considering the first point, you probably wouldn't need to use position to handle the positioning of the texts, perhaps just adjustments using marigin-top and margin-bottom.