Skip to content
OverviewSolutions
24
Comments
15
Kirsten ✨
@ofthewildfire

All comments

  • Kirsten ✨•460
    @ofthewildfire

    I see the following from my screenshot generated on the site and I am working on them and will update accordingly.

    • The background image quote is in the wrong spot on the screen. Which is odd, because when I click the link its in the correct position.
    • The lack of an h1 heading is causing an error on this site, I dont know where I would put an h1 header, I will look this up and ask some questions.
    • My solution is too tall, I do not like it, I will edit this.

    This will be done AFTER my base apparel, since the focus for me is on fixing my newbie challenges and this was after all just to practice laying out on a Grid

    Thanks!

  • nade12209•70
    @nade12209

    QR CODE

    #accessibility#airtable#cube-css#backbone
    2
    Kirsten ✨•460
    @ofthewildfire

    Welcome! Looks like its your first challenge

    Looks good, however it should be centered, you can do that easily by just using Flexbox on the body. I also noticed that your class names, have no meaning, a class name should have some meaning to the element.

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

    Your QR code is also missing an alt tag.

    Good luck!

  • Kirsten ✨•460
    @ofthewildfire

    Hi there. Your solution looks great, I just have a few small suggestions.

    When you zoom in (as some users do on sites), your component is chopped off a bit at the top, this is because there is no space for it to "go" - to fix this add a min-height: 100vh as opposed to a height - those two confused me to heck and back, but they are different :)

    An updated solution would be something like this:

    body {
    max-width: 90rem;
    display: flex;
    justify-content: center;
    align-items: center;
    min-height: 100vh;
    background: var(--Off-Black);
}

    Your .container element. First off, using a div is likely not the best option. Every page needs a main element, it is the entry point, and since this is just one component, its the place to use main -> You also are using a fixed width for your card, as a general rule fixed widths are not the best / actually most times you should avoid it. Instead, it would be better to use a max-width with a rem value. eg. max-width: 25rem to have the save consistency. Height value set to also is also not needed and does not add anything.

    The use of position is also so finicky and in my experience just makes it hard as all heck to make things response, and again its not neccessary, HTML will work with you to get all the proportions right, adding extras in this regard is just more complexity than is needed!

    An updated rule set for the container element would be something like:

    .container {
    max-width: 25rem;
    background: var(--Dark-Grey);
    border-radius: 20px;
    text-align: center;
}

    Adding a width to your image is not needed at all. The margin bottom is also not needed at all.

    Within your solution you use a lot of px values for your font sizes, this is not the best idea, you can read more here -> Why fonts should never be in pixels - so changing these to the recommended rem would be best!

    We also don't need the padding top and bottom on the .container p selector, I think you did it to add space, you can use something like Grid and gap for that!

    You'd add the Grid to the .container element, so, something like this: ->

     .container {
    max-width: 25rem;
    padding: 2em;
    background: var(--Dark-Grey);
    border-radius: 20px;
    text-align: center;
    display: grid;
    gap: 1.25rem;
}

    You can likewise do the same within your ul element to add space between your li elements.

    Overall, it was a good solution. Hope some of this was helpful!

    Happy coding!

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    Hiya

    Great solution, however, I think it would be an improvement to adjust the unit used in your max-width on your blog__card :) Currently you are used a fixed max-width, which means when you zoom in, the card gets squished and does not adapt! To resolve this using a unit value such as rem would be way better.

    .blog__card {
	max-width: 20rem;
}

    I also think that changing the <h2> to an <h1> might server accessibility a bit better, it is the heading and that seems important.

    Overall a really good job 🙏🚀

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    Great solution 👌💯

    My only advice would be to center the blog card on the page to more closely match the design.

    You did amazing however!!

  • Kirsten ✨•460
    @ofthewildfire

    Hmm. I'm on mobile right now but the challenge doesn't actually have any CSS to it. It's just a plain HTML file.

    I peeked as well I could on mobile at your GitHub repo. You should check the pathing to the required files is in check. I see you've said your CSS is in "assets" folder.

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    Hi! Your solution look really great! :)

    I only have one suggestion, your .result and .summary on desktop are not even, since you're using Flexbox, you can add a flex-basis: 50% to each of them so they are equal. On just the desktop version!

    PS: In case you care about the color being perfect, which is totally not needed, the button hover state is actually the gradient given for the purple card! I do prefer your color though. :)

    Your height looks fine to me as well. I also struggle not knowing when to use Flexbox or Grid, I usually end up using Grid, since I tend to like it more these days. :)

    Anyways, hope this was helpful, and happy coding!

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    Hi! Good solution, I just have a few things I think could be helpful

    • While you do import your google font, you actually haven't used it anywhere, you'll need to use it as a font-family: "font-name"; with the elements you need to apply that font to. Eg. font-family: 'Red Hat Display'; would be one of them, when you copy-paste the link, it also provides the names of the fonts on the Google interface in the sidebar! :)

    • Within your .container class, there is no real need for the margin-top/margin-bottom, in my experience using margins is kinda a way to confuse a situation. To center the card, you can use flexbox on the body, eliminating the need for the margin hack. <3

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

    • Within your .container class, you're using fixed width and this makes it not a good viewing experience on mobile/smaller screen, currently your container is set to 450px, so, if I shift to mobile it has over-scroll, in general fixed-width items is no bueno :) - a fix for this would be to use a max width for now and maybe do further research on width in CSS for responsiveness. This will help it flow better though> max-width: 400px and also, removing the height on your .container would go a long way. Height is not needed. :)

    • Removing the width on your container, will mean that your image will be out of the container, images should be able to resize with their content, so, to fix this, adding a width to the image would be very beneficial max-width: 100%;

    • Finally, not really needed, but, I think it looks better, adding a flex-wrap: wrap on your .two class div so that when the screen is smaller it wraps to the next line and looks neater might be helpful.

    PS: If you want to not make it flush against the edges, and add some space. Which I guess some people like, adding a margin: 0 1em; to the container would do that! :)

    Overall, a great solution and I hope this was helpful~

    Happy coding! :)

  • Kirsten ✨•460
    @ofthewildfire

    Hi! Your solution is really great

    • In terms of the responsive design, you're correct, for this small component, what you've done with the max-width trick is more than enough, its all you needed to do.

    I only have a few suggestions, you currently have a height: 100vh on your .container - this unfortunately cuts off the content when you zoom in, changing this to a min-height: 100vh would solve this. You are also using display: flex in your container class, which means your alignments using align-items and justify-content is handling the centering both vertically and horizontally, making your margin: 0 auto redundant.

    I hope this helps, its a perfect to me solution though. Well done~ :)

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    Hey! First off, fantastic job, it looks amazing, I just have some small changes I think might be helpful.

    1 - Your music icon image is stretched, its in a flex-container so we can solve that by adding a align-items: center to align the items vertically. This distributes the whitespace and makes it nice and centered

    .plan-container {
    width: 100%;
    height: 20%;
    display: flex;
    flex-direction: row;
    align-items: center;
    padding: 2vh 3vw;
}

    2: Within your price-plan container you used a span and a paragraph and that is fine, totally, however, a paragraph has default margin and that default margin is causing a lot of space above it and making it look way too wide from the span. This can easily be fixed by removing the top/bottom margin on the paragraph element.

    .price-container > p {
    color: hsl(224, 23%, 55%);
    margin: 0;
}

    -- and alternatively, you could also have used a ul/li to create this and it would have taken care of itself, seeing as <li> ... </li> are in-line and the flex-container would have nicely spaced all components out, but that is just personal opinion really. You did aweome whichever method you prefer. --

    3: Your button and anchor tag hover states are missing the cursor: pointer to give it that micky-mouse hand on hover like your "Cancel order" element has. So adding cursor: pointer would just be it

    You did a really great job and I hope these are helpful additions. :)

    Happy Coding ~ Kaycee.

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    A good attempt;

    however, paying more attention to the details to make it look as best to the design I think is needed, particularly in these spaces:

    • the corners are rounded in the design so adding a 10px border-radius on both the card and the QR image would be better, its too sharp in yours (Mind, it might be less or more than 10px, but I am just spit balling here)

    • Your font is not actually being imported, may I recommend this resource to see where you've gone wrong there. https://www.w3schools.com/css/css_font_google.asp

    • Again, with the design, following the style-guide, the font weights for both the header and the paragraph need to be different, so do the colours. :)

    Hope that helps. :)

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    You did a really good job, however, i would recommend, and this was probably just a typo on your part changing your first class margin settings to reflect this:

        width: 280px;
    background-color: hsl(0, 0%, 100%);
    border-radius: 20px;
    margin: 0 auto;

    instead, so that the card is centered on the screen.

    Also, without pro, the heights/widths are just eye-balling, but truth be told, your eyeballing was spot on for the vast majority of the challenge. Good luck on your future challenges.

  • Kirsten ✨•460
    @ofthewildfire

    Your image works on Github pages, I loaded it just fine; I thought I would mention it. You might have already fixed it though. It might be on your side/or some kind of issue locally only you are seeing.

    Challenge related things; the only thing i notice is the colour of the main header being slightly darker, but otherwise it looks perfect to me.

    Marked as helpful
  • Kirsten ✨•460
    @ofthewildfire

    Save the color (the greyish color) and the header not being a heavier font-weight, which I would recommend you follow though (in the ReadMe style guide, of course, you know about that) it looks good. The HTML and CSS are also so clean and I'm jealous. ;) - you gave me an idea on how to re-do mine too! ^^

    Well done. <3

  • Kirsten ✨•460
    @ofthewildfire

    Great job! :)

    Marked as helpful