Skip to content
OverviewSolutions
34
Comments
18
Ben
@BenConfig

All comments

  • Ben•770
    @BenConfig

    Hi, nice work!

    To stop the page from refreshing, you have to prevent the default behaviour on form submission.

    Try adding this JS:

    const form = document.querySelector('form');
form.addEventListener('submit', event => {
  event.preventDefault();
});
    Marked as helpful
  • Ben•770
    @BenConfig

    The 'dots inside the zeros' are a feature of the Space Mono font family. It looks like you are using the Roboto font family which does not have the dots.

    Why not just use the character '$' and then you can apply the color property?

    You should try building the page for mobile first. The mobile design is usually much simpler than the desktop design so it makes sense to start with that. You then apply media queries when you have more viewport space to work with.

  • Ben•770
    @BenConfig

    Remember to use <p> elements for blocks of text. <div>s and <span>s aren't semantic elements, they are just used to separate elements for styling purposes.

    For example, your .description element can be a <p>. And the .price element can be a <p> also in my opinion:

    <p class="price">
     <strong>Annual Plan</strong>
     <span>$59.99/year</span>
</p>
  • Ben•770
    @BenConfig

    Try to use the semantically correct HTML elements.

    For the .container you could use a <main> element instead of a <div> and your .cards could all be <section> elements instead of <div>s.

    Also you have used <p> elements for your .card-headings, but these are headings, so why not use <h2>?

    Marked as helpful
  • Ben•770
    @BenConfig

    Hey, I checked your page in Safari. There is a lot of extra space above the <h1> and on larger screens the text content starts to disappear from the bottom of the .text-box.

    For some reason, removing align-items: center; from .card fixed the issue but I couldn't figure out why.

    Marked as helpful
  • Ben•770
    @BenConfig

    How about this:

    .review::before {
    content: url(./images/icon-star.svg)
             url(./images/icon-star.svg)
             url(./images/icon-star.svg)
             url(./images/icon-star.svg)
             url(./images/icon-star.svg);
    margin: 0px 30px 0px 20px;
    display: inline-flex;
    gap: .25rem;
}

    You can use the gap property to increase/decrease the space between the stars as you wish.

    Marked as helpful
  • Ben•770
    @BenConfig

    This is really nice. I like how the elements come in from the sides as you scroll down. I really need to learn how that works.

  • Ben•770
    @BenConfig

    Hey, well done on this challenge, it looks quite tricky.

    1. The contact button doesn't stay focused because of href="#contact". I think this is linking to something which doesn't exist on the page which is removing focus from the link itself. If you change the attribute to href="#" it will fix the problem. I did notice that you have duplicated every property for the :hover and :focus styles. I think you can remove them all except color and background-color, since these are the only properties that actually change?

    2. I think the problem you're having is getting the text to stay at the bottom of the container? You could use grid to fix this. On .content-with-cherry change display: block; to display: grid;. Remove align-items: center; and add align-content: end;. Add some padding-bottom so the text isn't sticking to the bottom. Then do the same with .content-with-orange.

  • Ben•770
    @BenConfig

    Hey, well done on this challenge, it's a very nice solution.

    For your card icons, you have declared background-repeat: no-repeat; and background-position: right; on each one. Instead you could add those to your .card-icon class and remove them from the individual classes which will save you 10 lines of code.

    Also you can remove width: 100%; from .card-icon because it will take the full width of the parent by default, and text-align: end; doesn't appear to do anything so that can be removed too.

  • Ben•770
    @BenConfig

    Hey, I tried this and it fixed the issue:

    .hero-content {
    isolation: isolate;
    z-index: 1;
}
    Marked as helpful
  • Ben•770
    @BenConfig

    This looks pretty good. I would perhaps try to simplify the HTML as there seem to be some unnecessary elements.

    Maybe something like this:

    <main>
    /* Card 1 */
    <section>
        <h2></h2>
        <p></p>
        <a></a>
    </section>

    /* Card 2 */
    <section>
        <h2></h2>
        <p></p>
        <a></a>
    </section>

    /* Card 3 */
    <section>
        <h2></h2>
        <p></p>
        <a></a>
    </section>
</main>

    And the images are just for decoration so they can be added using a ::before pseudo-element on each <section>, instead of an HTML element.

    Marked as helpful
  • Ben•770
    @BenConfig

    I like the animation on the cards. That's really cool.

  • Ben•770
    @BenConfig

    Hi Nick, the curved background is an svg image. Check the images folder that came with the download. You have one for mobile: pattern-background-mobile.svg, and one for desktop: pattern-background-desktop.svg. Add this as a background-image to the body.

    Marked as helpful
  • Ben•770
    @BenConfig

    Hi Ildrim, well done, your page looks good!

    I believe you can remove the second no-repeat on your background-repeat property. It only needs to be declared once as it will apply to both background images.

    If you wanted, there is a background shorthand property that combines all of the individual background properties you have used. Although the way you have done it might actually be more readable.

    Regarding your second problem, I think that is just the default behaviour of flex (and grid), when applying justify-content: center, i.e. the element will be compressed as much as possible even if it is a block level element. I think width: 100%; would fix it?

  • Ben•770
    @BenConfig

    Using position: absolute; on every element is not a good idea. You will see this is a problem when you try it with larger projects which need to be responsive.

    Almost all positioning can be done with a combination of grid, flex, padding and margin.

    Marked as helpful
  • Ben•770
    @BenConfig

    Hey Julian,

    You can add margin-top: auto; on your buttons which will push them to the bottom of the containers, however this will only work if you have declared flex (or grid) on the containers.

    This should do the trick:

    .col {
    display: flex;
    flex-direction: column;
}

button {
    margin-top: auto;
}

    To answer your second point, it's totally fine to set min-height on anything. Setting height however might cause overflow issues depending on how much content is inside the element.

  • Ben•770
    @BenConfig

    Have you tried experimenting with viewport units to position those background circles?

    The right edge of the top circle is about 50vw from the right edge of the screen, and the left edge of the bottom circle about 50vw from the left edge of the screen. This is true for mobile and desktop so you shouldn't need to use any media queries.

    You could do something like this:

    background: url("images/bg-pattern-top.svg") bottom 50vh right 50vw / auto no-repeat,
            url("images/bg-pattern-bottom.svg") top 50vh left 50vw / auto no-repeat,
            var(--clr-cyan);

    Getting the Y axis correct is a little more tricky but I just went with 50vh to keep it simple and it still looks quite accurate to the design.

    Marked as helpful
  • Ben•770
    @BenConfig

    Hi Riccardo, you can use the <details> and <summary> elements for the accordion. I found this video helpful for this challenge: 2 HTML Elements I Never Used!? (Details & Summary)

    Marked as helpful