Is my design too simple? I'd like to have feedbacks for my design.
I’d love to hear your thoughts on how I can improve this design/website. Is there anything you feel is missing or could be done better?
I’d really appreciate your feedback!
I encountered some issues with deploying the code to Azure/GitHub, so I decided to switch to Heroku due to its convenience.
I really need to learn about clouds :)
I would definitely make it more responsive.
What challenges did you encounter, and how did you overcome them?I had issues making it responsive.
What specific areas of your project would you like help with?I tried to make it responsive for different devices, I did it but whenever I inspect again, it stops being responsive.
Hi @yeeezzzus,
I've seen your solution and would like to share some thoughts:
I'm not sure what you mean by "whenever I inspect again, it stops being responsive". You’ve defined a media query:
@media (max-width: 600px)
This means 600px is the breakpoint where the layout changes. Are you having trouble with this behavior?
Could you clarify what you mean by "make it more responsive"? Your page currently has two layouts:
If you're referring to adding more breakpoints, you'll need to define where and how you'd like the layout to change.
That said, media queries aren’t the only way to achieve responsiveness. CSS provides other techniques, such as:
clamp(), min(), and max()em, rem, vw, and vhflexbox, grid, and aspect-ratioHope this helps!
Most proud of:
The hardest task by far was to make the cards size dimension exactly like the reference image. To do that i used fixed width values and positions: relative and absolute
What specific areas of your project would you like help with?I'd like to know if there's another way to make the cards sizes as close to the reference as possible, but without using fixed width values and position: relative, position: absolute
Hi @Yasmim-Coimbra,
I've reviewed your solution and wanted to share some thoughts:
For viewports larger than 1173px, you've defined the grid-template-areas as follows:
grid-template-areas: "daniel daniel jonathan kira" "jeanette patrick patrick kira";
This indicates that you're aware the Grid .container is divided into four columns. However, you haven't explicitly defined the width of each column. To maintain a structured layout, measure the width of each column and apply grid-template-columns accordingly. For example:
.container { display: grid; grid-template-areas: "daniel daniel jonathan kira" "jeanette patrick patrick kira"; grid-gap: 30px; grid-template-columns: repeat(4, 300px); /* Replace 300px with your measurements */ }
In this example, each column has a width of 300px. Since you've already assigned grid-area to each card, the next step is to remove the width properties you've applied to individual cards and the .container (e.g., width: 115%, width: 85%, or width: 82%). This will prevent them from interfering with the Grid layout.
Additionally, remove position: relative from .container and position: absolute from .patrick to ensure proper alignment within the Grid.
Hope this helps! Let me know if you need any clarifications.
What are you most proud of?
Successfully implemented an interactive rating component that dynamically updates the UI based on user input.
Improved code structure by organizing JavaScript event listeners efficiently.
What would you do differently next time?
Improve state management by using localStorage so that the rating persists even if the page refreshes.
Add a "rate again" button in the thank-you state so users can submit another rating without refreshing.
Challenge: Initially, clicking a rating button didn’t properly update the selected state, as multiple buttons could remain highlighted.
Solution: Used forEach to remove the active class from all buttons before adding it to the selected one:
ratingBtn.forEach((btn) => btn.classList.remove("active"));
button.classList.add("active");
This ensured only one rating could be selected at a time.
What specific areas of your project would you like help with?Are there any redundant lines of code in my JavaScript that could be optimized?
Does my CSS structure follow best practices? Could I improve it for better readability or maintainability?
Is there a better way to handle rating selection without using forEach to remove the active class from all buttons?
Hi @rukhulkirom,
I've reviewed your solution, and I'd like to share some suggestions for improvement:
Using a convention like BEM can make your CSS more structured and easier to maintain. For example, instead of having .card and .thankyou-card, you could use:
.card { /* Base styling */ } .card--thank-you { /* Modifier for thank-you card */ }
This keeps your styles consistent and scalable.
id AttributesYou assign ids to your cards but style them using classes instead. Since the ids aren’t necessary, I recommend removing them to avoid redundancy.
Instead of relying heavily on <div>, consider using semantic elements:
<article> instead of <div> for your cards.<img> elements inside <figure> (or <picture> for responsive images).<ul> with <li> for the .rating-btns—great! However, add type="button" to each .rating-btn for better behavior.<form>Instead of handling each button click separately, wrap your <ul class="rating-buttons"> and .submit-btn inside a <form>, then:
type="submit" on .submit-btn.If you're keeping the click event on each rating button instead of using a <form>, you can still make your JavaScript more efficient using Event Delegation:
ratingBtn.forEach((button) => {
button.addEventListener("click", () => {
ratingBtn.forEach((btn) => btn.classList.remove("active"));
button.classList.add("active");
selectedRating = button.getAttribute("data-value");
});
});
document.querySelector(".rating-buttons").addEventListener("click", (event) => {
const button = event.target.closest(".rating-btn");
if (!button) return; // Ignore clicks outside buttons
document.querySelectorAll(".rating-btn").forEach((btn) => btn.classList.remove("active"));
button.classList.add("active");
selectedRating = button.getAttribute("data-value");
});
✅ Why is this better?
Hope this helps!
When I initially looked at solving this it initially occurred to me to use a grid, although I wanted to see if I could get it right with flex box. It ended up being pretty simple to set up breakpoints to change only a few properties in order to get the layout to work in all 3 screen sizes.
What challenges did you encounter, and how did you overcome them?I was hard time realizing that the tailwind classes for adjusting position in a container, switch the direction they work with depending on the main axis of the flex box. Some of them work with the main axis and some with the perpendicular axis.
I ended up finding some helpful info in the tailwind docs.
What specific areas of your project would you like help with?I'd like recommendations on how to improve the semantic html elements used throughout the page and components.
Hi @ecarlste,
I've reviewed your solution and would like to share my thoughts:
<div> inside <main>, even though it's the only child. Consider removing it and applying the CSS classes directly to <main> to reduce unnecessary nesting.<section> works fine, but you could use <hgroup> instead, as it represents the page's headings.<section> could be an <article> rather than a <div>. Also, there’s quite a bit of <div> nesting inside the cards. You could simplify it, as many of these elements can stand on their own with some CSS adjustments.Hope this helps!
I was able to apply the skills that I've learned so far in regards to spacing, writing more clean CSS, and how HTML/CSS interacts with each other. I believe my solution is very close to the design.
What challenges did you encounter, and how did you overcome them?Getting the image sizing right was tricky, definitely an area of improvement / comfort I'd like to explore.
What specific areas of your project would you like help with?Any optimizations or suggestions whether it's to my code structuring or naming conventions would be greatly appreciated!
Hi @code269,
I've reviewed your solution and would like to share some thoughts:
Use <picture> for Responsive Images
Instead of using CSS to switch between <img> elements based on viewport size, you can use the <picture> element to handle this more efficiently, reducing unnecessary CSS.
Instead of this:
<img class="card__img-mobile" src="./images/image-product-mobile.jpg" alt="Chanel perfume bottle"> <img class="card__img-desktop" src="./images/image-product-desktop.jpg" alt="Chanel perfume bottle">
Use this:
<picture> <source media="(min-width: 768px)" srcset="./images/image-product-desktop.jpg"> <img class="card__img" src="./images/image-product-mobile.jpg" alt="Chanel perfume bottle"> </picture>
This approach makes your code cleaner, more semantic, and reduces CSS complexity.
You can enhance readability and structure by using more appropriate HTML elements instead of <div>:
card__content → Use <section> for better semantics.card__text → Use <p> if it represents a paragraph.card__subtitle → Use <span>, as it behaves as an inline element, making it a better fit for short text like tags.price-tag__bold and price-tag__small; using <span> is more appropriate than <div>, as they should remain inline.Hope this helps!
I was happy that I was able to complete the project. My frontend skills are moldy and this was a good challenge to brush them off. I am happy I was able to replicate the design.
What challenges did you encounter, and how did you overcome them?I had a bit of trouble the image being centered and the bottom half of the card being spaced properly. I googled around for padding and margins.
What specific areas of your project would you like help with?If there is a better way to deal with the padding/ margins. It felt a bit fiddly.
Hi @Taylor-McNeil,**
I've reviewed your solution, and I’d like to share my thoughts:
Centering the <img> using Flexbox
Since you used Flexbox to center <main>, you can apply the same approach to center your <img>. Just apply Flexbox properties to its parent (.qr-container-img):
/* Make the parent a flex container */ display: flex; /* Center the image inside */ justify-content: center; align-items: center;
Understanding margin and padding
You mentioned having trouble with spacing and found padding and margin to adjust the layout. However, it seems there’s some confusion about what they do. Here’s a clearer explanation:
padding, border, and margin.padding is the space inside the element, between the content and the border.margin is the space outside the element, creating distance from other elements.Visual Representation:
MARGIN (space outside)
┌───────────────────────────┐
│ │
│ BORDER (edge of element) │
│ ┌─────────────────────┐ │
│ │ PADDING (inside) │ │
│ │ ┌───────────────┐ │ │
│ │ │ CONTENT │ │ │
│ │ └───────────────┘ │ │
│ │ │ │
│ └─────────────────────┘ │
│ │
└───────────────────────────┘
Using this structure, you can better understand how spacing works and adjust it accordingly for the desired layout.
Let me know if you need further clarification!
Hope this helps!