P
TancrediPaterra• 40
@TancrediPaterra
Submitted
RogerTito455
@RogerTito455
All comments
- RogerTito455• 160
@RogerTito455
Posted
Your HTML and CSS implementation for the testimonial grid is well done! Here’s some feedback to help you refine it further:
HTML Feedback: Semantic HTML:
You're using div elements effectively, but consider using more semantic elements where appropriate. For example: Wrap the entire content in a <main> tag instead of a div with main-container. Each testimonial card could be an <article> element instead of a div with a card class. Alt Text:
The alt text for images is descriptive, which is good, but avoid using the word "icon" unless the image is purely decorative. Instead, use something like "Portrait of Daniel Clifford". Title Tag:
Replace [Challenge Name Here] with the actual challenge name or a more descriptive title, like "Testimonial Grid Section | Frontend Mentor Challenge". CSS Feedback: Use of CSS Variables:
Great job using CSS variables for your colors and typography. This makes your stylesheet easier to maintain and scale. Grid Layout:
The grid layout is well-implemented. However, in your media query for mobile, consider using grid-template-areas for a more intuitive arrangement, especially if you have more complex layouts. Typography:
Consider adding line-height to your text to improve readability, especially in longer paragraphs. Box-sizing:
You’ve correctly used box-sizing: border-box; in the .card class, but it’s generally good practice to apply this globally to all elements: css Copiar código *, *::before, *::after { box-sizing: border-box; } Accessibility:
To improve accessibility, ensure that there’s enough contrast between the text and background colors. For instance, you might want to check the contrast between the white text and var(--moderate-violet) or var(--very-dark-grayish-blue) backgrounds. Consistency:
There's a typo in your CSS variable names: --very-dark-blackish-blue-light-trasparency should be --very-dark-blackish-blue-light-transparency, and similarly for --very-dark-blackish-blue-hard-trasparency. Font Size and Units:
The use of rem instead of px for font sizes and margins/paddings is recommended for better scalability across different screen sizes and devices. Responsive Design:
Your mobile styles are well handled with a media query. However, consider using a mobile-first approach where the base styles are designed for smaller screens, and then using media queries for larger screens. This approach tends to be more efficient and easier to maintain.
0 - phucphan01866• 110
@phucphan01866
Submitted
RogerTito455• 160@RogerTito455
Posted
Hey there, well done!
My feedback:
HTML:
Semantic HTML: Consider using more semantic tags to improve accessibility and SEO. For instance, wrap the header section in a <header> tag, and the card container in a <section> tag. This helps search engines and assistive technologies understand the structure of your content better.
Alt Text: The alt attributes for images are empty, which isn’t ideal. Provide descriptive alt text that conveys the purpose of the images. For example, "Supervisor icon" or "Icon representing Karma" would be good.
Class Naming Conventions: While the class names like .Post1, .Post2, etc., work, consider using more descriptive names that indicate their role or position, such as .card-supervisor, .card-team-builder, etc. This can make your code more readable and maintainable.
Avoid Inline Styles: The attribution section at the bottom uses inline styles. It’s better to move these styles into your external CSS file to keep the HTML clean and maintainable.
CSS:
CSS Reset: It’s good that you’re setting margin and padding to zero. Consider using a more comprehensive CSS reset or normalize file to ensure better cross-browser consistency.
Box-sizing: Consider adding box-sizing: border-box; to the universal selector (*). This helps to make width and height calculations more intuitive by including padding and borders within the element's total width and height.
Font Import: You’ve correctly imported the Poppins font. Make sure to include a fallback font, like sans-serif, in your font-family declarations to ensure your text displays properly if the custom font fails to load.
Grid Layout: The grid layout is well-implemented, but the use of grid-template-rows: auto auto auto auto; in the card container might be redundant since all rows will automatically take up the necessary space. You can simplify this by removing the explicit row definitions.
Media Queries: You’re using a media query to adjust the layout for larger screens, which is great! However, consider a mobile-first approach where the default styles cater to smaller screens, and media queries adapt the layout for larger screens.
Color and Typography Classes: The use of utility classes for colors and typography (like .Red, .Cyan, .bold, .darker) is fine for small projects. However, in larger projects, this can lead to cluttered HTML. Consider moving these styles into component-specific classes or using a CSS preprocessor like Sass for better organization.
Padding and Margin: You’re using padding and margin effectively to create space within and around elements. However, consider using relative units like rem or em instead of px for more responsive and scalable spacing.
Button and Link Hover Effects: Although you don't have any interactive elements like buttons in this code, it’s good practice to add hover effects or transitions to any interactive elements to improve user experience.
Use of Units: You’re using a mix of px and % units, which can sometimes lead to inconsistencies, especially with responsiveness. Try to use rem or em for consistent sizing that scales with the user’s default font size.
Overall, your code is well-organized and demonstrates a good understanding of CSS grid and responsive design principles. With a few tweaks, you can improve accessibility, maintainability, and responsiveness. Keep up the great work!
Marked as helpful
0 - ElkuchWaltz• 70
@ElkuchWaltz
Submitted
What are you most proud of, and what would you do differently next time?
I'm proud that I was able to get my design to match well with the mobile and desktop prompts using the media query, which I hadn't used before. I hope next time I am able to do a project like this faster.
What challenges did you encounter, and how did you overcome them?
Media queries, selecting different images based on the media query, and using different alignments within the same container were all new for me. I read articles linked by Frontend Mentor, googled some specific questions, and perused the code of other solutions to try out different ways to incorporate the code in my design.
What specific areas of your project would you like help with?
Inefficiencies in the code Best practices that are missing or done incorrectly
RogerTito455• 160@RogerTito455
Posted
Hey there, well done!
My feedback:
HTML:
Great use of the <main> tag: You've correctly wrapped the core content of your page, which is great for accessibility and semantic structure.
Alt Text Improvement: The alt text you've used is descriptive, but it could be more concise and focused on its purpose. Consider simplifying it to something like "Gabrielle Essence Eau de Parfum bottle" since the image visually represents the product. If the image is a link, mention where it leads.
Use of <button> inside <a>: While it's generally fine to wrap a <button> inside an <a> for styling purposes, it's better to apply the styles directly to the <a> tag when it's used as a button. This helps keep your HTML semantically correct, since <button> is usually for form elements or JavaScript actions, and <a> should handle navigation.
Ensure unique IDs: If you plan to add IDs to elements in the future (not currently in your code), make sure each ID is unique on the page. Using classes for styling, as you’ve done, is a better practice when multiple elements share the same styles.
CSS:
CSS Reset: You've correctly implemented a CSS reset, which is excellent for ensuring consistency across browsers. You could consider using a more comprehensive reset like normalize.css for additional cross-browser compatibility.
Font Family: You have a good handle on specifying fonts. Just a small suggestion: it’s good practice to add a generic font family at the end of your font-family declarations as a fallback, like you did with sans-serif.
Padding on the body: You've included a margin on main, which is good. Adding a bit of padding to the body (e.g., padding: 1rem;) can ensure that content doesn’t touch the viewport edges on smaller screens, providing better aesthetics and readability.
Image Responsiveness: You’ve done a great job ensuring images are responsive with max-width: 100%. Additionally, setting display: block; on images (as you did) helps remove any unwanted whitespace beneath images, which is a common issue.
Flexbox Usage: Your use of Flexbox in the card layout is spot on. It makes the layout responsive and easy to manage. However, consider using gap in the .price container instead of margin-left on .srp for spacing between the elements, as it’s more straightforward and keeps spacing consistent.
Mobile-first Design: It’s great that you’re using media queries effectively! Remember, a mobile-first approach is often recommended—starting with styles for smaller screens and adding media queries for larger screens.
Consistency in Units: You’ve used rem units for most sizing, which is excellent for scalability. However, you might want to ensure that all sizes are consistent, even in media queries (e.g., use rem instead of px for breakpoints if possible).
Hover States: The hover effect on your button is a nice touch. Consider adding subtle transitions (e.g., transition: background-color 0.3s ease;) for smoother visual feedback.
Specificity in Selectors: You’ve already used specific class selectors, which is good for maintainability. Avoid deep descendant selectors like .text-container h2 (though you haven’t used this specifically), as they can become hard to manage in larger projects.
Marked as helpful
0 @iCloudBMX
Submitted
RogerTito455• 160@RogerTito455
Posted
Hey there, great job on the code! You've done well in structuring your HTML and CSS. Here's some feedback to help you polish it further:
HTML: Main Element:
You've done well wrapping the content in a <main> element, which is excellent for accessibility. However, make sure that the <header> and <footer> elements are placed outside of <main> if you had them. This practice helps screen readers better understand the structure of your page. Image Alt Text:
The alt text for your image could be improved for clarity and accessibility. Instead of "Omelette picture," consider something more descriptive like "A plated simple omelette." Avoid phrases like "picture of" because screen readers already announce that it's an image. Typo Corrections:
There are a few typos in the HTML content. "Preperation" should be "Preparation," "tdis" should be "this," and "buffer" should be "butter." These small errors can detract from the professionalism of your page. Consistent Tag Usage:
You're reusing the recipe-page__ingredients class for multiple sections. It's better to use a unique class for each section, like recipe-page__instructions and recipe-page__nutrition, to avoid confusion and improve maintainability. CSS: CSS Reset:
Good job including the CSS Reset! It ensures that your styling is consistent across different browsers. Font Family Fallback:
While you have specified the correct font-family, adding a fallback font ensures your text remains readable even if the primary font fails to load. Modify the line to: font-family: 'Outfit', sans-serif;. Body Padding:
I like that you've centered the content using flexbox, but adding a padding of 1rem to the body will ensure that the card doesn't touch the edges on smaller screens. Min-Height on Body:
Instead of setting a fixed height: 100vh; on the body, use min-height: 100vh; to prevent content from being cut off if it exceeds the viewport height. Rem Units for Responsiveness:
You've used rem units effectively, but the width: 30%; on .recipe-page can be more consistent with other sizes by setting it to max-width: 20rem; to maintain a more consistent look across different screen sizes. Image Styling:
On the image, you've used aspect-ratio which is great. Add display: block; and change width: 100%; to max-width: 100%; to prevent the image from overflowing its container. Class Naming and Selectors:
Consider adding specific classes to elements instead of relying on descendant selectors like .text-container h2. For example, use .recipe-page__title directly on your <h2> to improve readability and maintainability of your code. Media Queries:
You've included a media query for screens smaller than 1024px, which is good practice. Ensure you're using rem or em for media query breakpoints rather than px for better scalability. Miscellaneous:
Some CSS properties could benefit from additional tweaking, like reducing the padding inside .recipe-page__header__time or increasing the gap between list items for a cleaner look.
Marked as helpful
0- Nandakishor-M• 30
@Nandakishor-M
Submitted
What are you most proud of, and what would you do differently next time?
I feel proud of learning more things of CSS. Next time planning to use bootstrap.
What challenges did you encounter, and how did you overcome them?
I faced challenge when hovering the div especially the way the background color appearing. I solved it using transition property.
What specific areas of your project would you like help with?
Feel free to provide feedback on any error code and any useful tip to make my code more perfect.
RogerTito455• 160@RogerTito455
Posted
Semantic HTML: The HTML is fairly semantic and well-structured. However, you might consider using more specific tags like <nav> for the social media links since they relate to navigation.
You could also add a <footer> tag even if it's empty for now, which would give the page a more complete structure. Accessibility:
Consider adding aria-label attributes to the social media links to improve accessibility, especially for users who rely on screen readers. Ensure the images have more descriptive alt attributes. Instead of just "person-photo," something like “Profile photo of Jessica Randall” would be more meaningful. Links:
The social media links all have the same visible text. You could add a title attribute to the links to provide additional information on what happens when clicked. CSS:
CSS Variables: You might want to use more CSS variables for colors, font sizes, and other repeated styles. This will make future maintenance easier. Grid Layout: The use of display: grid; is good, but you could specify grid-template-columns outside of the individual divs to create a more controlled and adjustable layout. Transitions: The transition time (transition: 2s) is quite long. Generally, users expect quick transitions (between 0.2s and 0.5s). Additionally, specifying exactly which properties should transition (e.g., transition: background-color 0.3s ease) can improve performance. Responsiveness:
While the site adapts to different screen sizes, you could improve the mobile experience further. For example, using media queries to adjust the size of the card and text on smaller screens would be beneficial. Bootstrap Usage:
Although you've commented out the inclusion of Bootstrap, if you're not using it, you could remove that line entirely to reduce file size and loading times. If you do decide to use Bootstrap, you could leverage its utilities to further simplify your CSS.
Marked as helpful
0 - Gauthier Duffier• 20
@gaut123456
Submitted
RogerTito455• 160@RogerTito455
Posted
I think that your code is really great, you made it like the example but I think that you should be more exactly with the margins and the paddings. But in general I think that you make a great job. Well done! :D
1 - thamu-acn• 200
@thamu-acn
Submitted
What are you most proud of, and what would you do differently next time?
I was able to produce the layout on the design. My page is responsive which makes it adaptable to different screen sizes without distorting content.
What challenges did you encounter, and how did you overcome them?
Initially the image couldn't fit inside the container div until I set max-width to 100%.
What specific areas of your project would you like help with?
I would like to know if it is right to add bootstrap to this project and use the card component to avoid writing many css styles
RogerTito455• 160@RogerTito455
Posted
From my point of view I think that the code is well-structured, readble and reusable since you have used all of the correct components of css. However I think that there are other ways of making that but I think that the code is similar of the example. Well Job!
0
