Skip to content
OverviewSolutions
9
Comments
7
d8701a
@d8701a

All comments

  • d8701a•240
    @d8701aPosted about 1 year ago

    Just try to center the text in your sections, especially headings. You can achieve this by setting the whole section to display: flex or you can try to center it with margins or translate method, it's up to you.

    Also try to use picture instead of img elements for the cherry and orange pictures. Using the <picture> element is great because you can use images of different dimensions depending on the screen size. In your case, I think you used the image intended for a mobile version for your desktop version as well. For example, if you compare the mobile version of the orange picture, you see it has more height than the one made for desktop, that one is shorter.

    The rest of your project looks good, keep up the good work!

    Marked as helpful
    1
  • David Tejada•380
    @david-tejadaSubmitted about 1 year ago

    I just completed a challenge 🏅, any feedback is welcome 💬

    • I wanted to add a bit of pizzaz so I made the card flip to show the thank you state. What do you think about it?
    • I tried to put special focus on accessibility but any improvement suggestions are welcome.
    • Any other suggestions are welcome.

    Flipping interactive rating component

    1
    d8701a•240
    @d8701aPosted about 1 year ago

    Hello and congrats on completing this challenge!

    The flipping card idea was awesome, it looks smooth and much more elegant than simply replacing one card with a thanks-card, although both solutions are correct and acceptable.

    You chose a rather unusual solution, with rating buttons 1-5. Most of the solutions I saw here were just adding a button element in HTML and then styling it as necessary in CSS. You decided to go with radio buttons instead, which maybe generates a bit more HTML code but in the end, if it gives you a desired result - go for it!

    Your code seems alright to me. Maybe try using more descriptive names for icons and images. For example: <div class="star"> <img src="images/icon-star.svg" role="presentation" /> </div>

    If someone else gets to work on your project, the code is more readable and understandable if the classes have descriptive names. For example star-icon would be more appropriate in this case, so anyone looking at your CSS will know what this class does. If you only put div class = star, it's hard to see without checking the HTML if that star is an image, a logo or an icon.

    However, your solution is very interesting, your code looks good and I think you are definitely on the right path, just keep it up! :)

    Marked as helpful
    0
  • Djengis05•30
    @DarkGamerXDOFFSubmitted over 1 year ago

    I've had most problems with centring the card. I ended up using flexbox for this but was this the best solution? Also, what would be a more efficient way to manage the card size?

    QR Code Component using basic HTML and CSS

    2
    d8701a•240
    @d8701aPosted over 1 year ago

    Hi!

    Using flexbox to center elements or for layouts in general is a good idea. So you made a good call when you decided to use it in this case. There are other ways of horizontally centering elements, such as using margins: (margin: 0 auto), using transform: translate(-50%, -50%) but this also requires an element to be absolutely positioned so I'd avoid this one, using grid and of course - using flexbox. For a simple layout like this, flexbox seems like a perfect use case.

    When it comes to managing the card size, I believe the priority should be its responsiveness. It should be able to change it's dimensions depending on the screen size. For this, try learning more about setting width with one of these three things: min(), max() and clamp() function. For more information on these, check this article: https://web.dev/min-max-clamp/

    0
  • d8701a•240
    @d8701aPosted over 1 year ago

    Hello and congrats on completing this challenge! I checked your code and overall, it looks very good to me. It's clean and precise, also you didn't repeat yourself, so that's also great.

    I noticed two things, so I'll write them here, but they are not mistakes, just maybe little improvements to be made.

    • First: try using semantic HTML elements instead of generic containers. So you could replace <div class = "main"> with simply <main> or add a class to it, for example <main class = "yourclasshere">.

    There are a couple of reasons why this is better and one of them is that <main> already exists in HTML, so you can use it. It improves accessibility of your website, code readability, and can definitely improve SEO ranking for your website as well. Another semantic HTML elements are: <nav>, <article>, <aside>, <section> etc. Try integrating them into your code and give them priority over custom div elements whenever possible.

    • And second thing - try to avoid using absolute units such as pixels. They are acceptable for setting paddings and margins (although some people use rems or ems for them as well), but for font-size try to use rems. They improve mobile responsiveness and overall using absolute units is a bad practice now.

    Other than that, awesome use of flexbox, awesome use of width and max-width on a container and overall, just keep learning, you are on a good path!

    Marked as helpful
    1
  • d8701a•240
    @d8701aPosted over 1 year ago

    Hello and congrats for completing this challenge!

    Now to answer your questions, you did add a box-shadow and that's completely fine, as a matter of fact your version looks more polished, and I like it. I don't think, however, that you needed to add box-shadow property at all, I believe this card doesn't have it set, but in the end, it really is a matter of preference, not a mistake. You can simply remove it or decide to keep it, both ways are fine.

    When it comes to centering the image in the mobile view, it's probably due to margins and padding. I can see in your code that you used the mobile-first approach, and that's commendable. So you set a media query for screens larger than 400px where you change the width of the container, but you didn't change all the paddings and margins. Usually, when we resize the size of a container/wrapper, margins and paddings need to be adjusted as well.

    Also if you want to have better control of images, make them responsive etc., you can try wrapping them inside a .div, setting dimensions on that .div and then just telling the img to take the whole width of that .div. Like this:

    <div class = "image__container"> <img>your image goes here</img> </div>

    And in CSS:

    .div { width: 420px; height: auto;

    img { width: 100%; height: auto; (if you want an image to preserve an aspect ratio) object-fit: cover}

    This way you are telling the img to take 100% of its parent container, which is the .div container and it's width will be 420px. In case you decided to set the width of your image to 50%, then it would take only 50% of 420px. It makes images responsive and easier to manipulate.

    Also by limiting the container size, your image for this project would've been smaller.

    Other than that, I see you are already familiar and comfortable using custom properties/variables and rems as relative units which is awesome! Keep up the good work and just keep on learning!

    0
  • 3p0na•20
    @3p0naSubmitted over 1 year ago

    Hi all,

    would it make sense to use sub-divs (one for the qrcode, one for the text...) or would you say, one div for the whole container is enough - like in my solution? I thought using more than the one div could maybe make the code unnecessarily complicated.

    Thanks for feedback.

    Display QR Code

    1
    d8701a•240
    @d8701aPosted over 1 year ago

    Hello there! Congrats on having completed this challenge, awesome work!

    To answer your question, yes, definitely you could use sub-divs. I saw many developers use them.

    So your whole card could have been wrapped in a .box_wrapper div. Then inside, you could've created .image_wrapper and .content_wrapper. Inside the .image_wrapper would be the QR code image, and within the .content_wrapper you could have put the h2 and p elements with their content. As a matter of fact, this is how I did the challenge.

    To summarize, everything would be contained in a .box_wrapper, which would be the parent container, and you could set it to display: flex, set its width etc.

    Marked as helpful
    1
  • 04leslie•20
    @04leslieSubmitted over 1 year ago

    I found it difficult to position the QR_code exactly at the center of the page, although i watched many YouTube videos. Waiting for your corrections, all feedback welcomed!!

    QR_ code challenge using div and container

    2
    d8701a•240
    @d8701aPosted over 1 year ago

    Hi, congrats on finishing this challenge!

    I took a gander at your code and here are a couple of things that perhaps I would've done differently:

    • Avoid setting many properties on body. Somehow I prefer to keep it clean, I might set font-family on my body but not much more. Some people put display: flex or display: grid on body element (I saw you put grid), but generally I think it will be easier for you as a beginner to treat body element as the whole working space and not limit its size or height.

    • Instead, I would use another element within the body and wrap my content inside. For example, you can call it <main> or make it simply a div wrapper and put your content inside. Now here is where you can set your viewport height: 100vh, display: flex or grid properties, limit its width etc.

    • There are many ways to center elements vertically or horizontally but I think the simplest solution is display: flex. Now you might want to keep in mind that display: flex must be set on a parent container, not the children inside of it, in order to make it work. Then you can use justify-content: center and align-items: center properties to perfectly center it.

    Let me show you what I mean:

    <body> <main> <div class="box"> <img>Your image goes here</img> <h2>Heading</h2> <p>Paragraph</p> </div> </main> </body>

    In CSS, that would look like this: main { display: flex, justify-content: center; align-items: center, min-height: 100vh; width: 100%; }

    .box { display: flex; flex-direction: column; justify-items: center; align-items: center; width: 290px; }

    • Last but not the least, I saw you used grid on the body element. While some people do this for responsive layouts, I prefer to avoid it, like I said, it's much cleaner if you create another section within the body and set the properties to it instead of the body. Your grid property will not do much unless you define the columns too. So something like: display: grid; grid-template-columns: 1fr 1fr; could give you two columns etc.

    My advice is to spend some more time on flexbox and grid. Once you get it, it will make many things so much easier for you, for example centering and aligning items, responsiveness etc. Don't rush it, flexbox is a weird thing, it likes to mess around sometimes and I don't know if anyone really understands it 100% but you will be able to do so much more with the basics.

    I think you are onto something good, though. The idea to use grid to create a card is not a bad idea at all, I've seen some very experienced developers do it. Just don't forget to define columns with grid.

    Here is a video of a really amazing developer whose channel I've been following for a while and his content is nothing short of extraordinary. He explains flexbox and I hope it will be useful to you as well: https://www.youtube.com/watch?v=u044iM9xsWU

    Other than that, just keep learning and don't get discouraged, it's a process and it takes time but in the end does get rewarding! :)

    Marked as helpful
    0