Hey Guillermo, I think this looks quite well put together! I think you did many things well; for instance, I like how readable and well-annotated your components are. There are just a few things I noticed could be better.
I suggest you remove the up and down arrow for the text field when inserting a pledge (I was using Firefox to review your website - this styling can differ from browser to browser). Here is a good link regarding how to remove the arrows by W3School
When I click on 'Pledge with no rewards' - I'm presented with a text field which defies the purpose of continuing without having to insert the reward
[Optional] Last thing I want to outline was the fact that I could continue with an empty field (without inserting a price) - it would be a good user experience if I got a feedback that said I need to insert the $ amount
All in all, I really think you did a great job on the whole. You used proper elements for each usecases which something important in regards to readability and performance. As for me, I didn't use any extra state management tools except for what is basically provided by React (which is useState and useReducer). Hope some of this can help you out, and really great work on the whole, so keep going!
Hi BDWNAV, good job putting this together! 😊 I got a few suggestions for you here:
I would suggest wrapping the main-div inside the <main>...</main tag as this will semantically helps the code refer main content of the body.
Right now the component in the mobile have a stretched image with height that fills in the screen. This could restrict the flexibility of the component and one fix would be to wrap your card into a container class like as follow:
.container{
width: min(80%, 600px);
/*This will give the card a max width of 600px but will give the elements in it 80% maximum width of the screen (if its below 600px) */
margin-inline: auto; /*Centers the element */
}
.main-div{
min-height: 450px;
padding: 1rem;
/*Now you can remove the width constraint here and the 100% height & width property within the mobile view */
}
Instead of using the <image/> tag with CSS for handling the switching of the desktop and mobile images, try to use the picture element instead since this is a product image and is thus central to the card.
<picture>
<source media="(max-width: 375px)" srcset="images/image-product-desktop.jpg" />
<img src="images/image-product-mobile.jpg" alt="Glass perfume bottle placed against a leafy background" />
</picture>
Hope some of this can help you out, and really great work on the whole, so keep going!
Hi Oseremen, first off, I think you did a great job putting this together! If there is one thing I would change, it is centering the card in the middle and adding a background shadow to your container (card). Something like:
Making display property grid - transforms the element into a grid container. A grid container allows you to arrange its child elements into a structured grid layout. Then the place-items:center property centers the grid items both horizontally and vertically within the grid container. Lastly the min-height: 100vh sets the minimum height of the element to 100% of the viewport height (vh stands for viewport height). This ensures that the element's height will always be at least as tall as the visible part of the browser window, making it useful for creating full-height sections or components.Hope this helps you out a bit!
I handled this challenge quite quickly, struggled a bit with responsive dimensions of elements, but in the end, I nailed it!
I noticed incorrect display of my component on GitHub (missing shopping cart icon). Everything works in my code; GitHub likely has an issue with the path to the icon.
Hey Veronica, just came across your solution. Impressive - I like your solution and even better, I like your confidence. You did a very good job. A small suggestion though - in your <article> tag you gave it a fixed width of 21.4375rem, by doing this you are taking away the responsiveness and setting a constant width that stays the same for a certain condition (in your case responsiveness). Now there is no problem with this as long as it gets the job done but its not as flexible as it should be. Here is a better solution for it:
article {
width: min(21.4375rem, 85%);
...
}
This will give it a max width of 21.4375rem but will keep the width at 85% in case its lower than the given width. By doing this you won't need to change or adjust the width property in different media size. Overall, you have done a pretty solid job for this challenge and seeing from your code I have no doubt that you will progress even better with time. Keep up the good work Veronica 👍🏽
Hi Tuminha, congratulations on finishing the challenge. You have done a decent job on making the landing page look as the design but I see a small technical mistake (in terms of semantics) and room for improvement in your code. For example, you used a <p> tag for a button in the hero section which is not semantically correct. Plus, the hover state of each button is not working.
Additional to this, the column for the card list is 2 while on the design is 3 (for the larger screen) and there is a bit of margin to the body. Not sure what is causing this but its recommended that you use a reset css before you start styling. Here is a reset link in case you are interested in learning more about it - https://piccalil.li/blog/a-modern-css-reset/ - what it does is it basically reset the default browser style so that you build every style per your wish (I find this to be very helpful).
Although, I'm giving you suggestions on improvement I still think you have done a pretty solid job finishing this challenge and I think you are just starting out. I wish you a good journey of building up yourself 💪🏽
Congrats on finishing the challenge @mritul 😊, I was going to check your code along with your website visually but the link to the source code (github link) is not working☹️. I also noticed that the images (icons) on the website was not loading - I think the path of the images is not correct.
Congrats on finishing the challenge 🎉. I have to say the choice between using width: 100% and max-width: 100% for the <img> tag depends on what you want to achieve with your images and how your layout is structured.
width: 100%: This CSS rule will make the image take up 100% of the available width of its containing element. If the containing element's width changes, the image will scale accordingly.
Note that this can lead to image distortion if the image's aspect ratio is different from its container (since 100% refers to the parents width).
max-width: 100%: sets the maximum width of the image to 100% of its containing element's width. This is commonly used to ensure that the image doesn't exceed its natural size and maintains its aspect ratio while resizing. It's a good choice for responsive design when you want the image to scale down to fit the container but not exceed its original dimensions.
Remember, using a <div> to wrap an <img> can be a helpful technique, especially if you want more control over the layout and the behavior of the image. For example, you can use a <div> to create a container for the image and then apply a max-width: 100% to the image inside the <div>. This can be useful for adding padding, margins, or other stylings to the image container.
Hope this helps you understand when to use width:100% and max-width:100% on imgs 😊
Hi Krishna, Seems like you are crushing the challenges 💪🏽. The way you used animation, the structure of your content, and responsiveness are all praiseworthy but I faced an issue while testing your website on a mobile device. The navigation menu was not showing up when I clicked on the hamburger menu.
Other than this, I say it is awesome 😊. Keep up the good work 👍🏽
Hi there Paul, this looks like a great start, and I think your code works fine. But I'm not sure why you went with table instead of div for this specific challenge but I take it, you've your own reason.
However, I do suggest that you use unordered list <ul> for the layout instead of a table as I believe its semantically wrong. According to MDN documentation, the <table> HTML element represents tabular data — that is, information presented in a two-dimensional table comprised of rows and columns of cells containing data.More info here.
You might also need to adjust the typography of the following element:
Your result (on the left side of the card)
of 100 (under the score - its too small)
Lastly, you might reduce the border radius on the button and also add a :hover property on it. Additional to this, add cursor: pointer on it as well.
Dear friends,
I am excited to announce that I have completed the coding project. I would greatly appreciate it if you could take some time to review my work and provide your valuable feedback.
Your feedback will help me improve my coding skills and ensure the quality of our projects. You can access the project " https://sensei-kaboto.github.io/PriceGrid/".
Hi there @Sensei-Kaboto, Congrats on finishing this challenge. I saw your solution and I have to say in visual terms, it is top-notch and deserves credit. In terms of your code, I believe you can improve naming your CSS classes. I suggest you check out the CUBE or BEM way of organizing your CSS classes. Going over your source code I also saw that you used inheritance in your CSS to achieve some of the styling - which is not recommended in a professional environment. It is widely recommended that you give each of your styles a class. Lastly, the line height of the body text on mobile might be better adjusted (they're too close). Other than these you did a good job. Keep it up 👍🏽
I found it hard to center the component on desktop screen without using the following CSS properties; position, top, left and transform. These properties made my work harder when making it responsive on smaller screens so I just omitted them.
Please, let me know how i can achieve centering the component without using those aforementioned properties.
Hi Caleb, Congratulation on finishing the challenge. It might be a bit difficult to center elements initially but I'm pretty sure that with time you will adapt to the best solution and patter. There are a couple of ways you could use to center an element but I will mention 2 (as I use them most of the time).
CSS Grid:
.grid-element{
display: grid;
place-items: center; /*This will center the elements inside the grid */
}
CSS Flexbox:
.d-flex{
display: flex;
justify-content: center; //centers the content on horizontal axis
align-items: center; //aligns the content at center vertically
}
Depending on how you want to structure your layout, you can use either of these methods. You can check more details about them from Kevin Powell on here: https://youtu.be/rg7Fvvl3taU?si=FSfiY-WPrGgTpUym
