Skip to content
OverviewSolutions
49
Comments
18
Brendan
@OneManBanned

All comments

  • Brendan•1,120
    @OneManBanned

    Hello Benjamin,

    • if you add min-height to the body it will center everything on the page.

    Nice solution.

  • Brendan•1,120
    @OneManBanned

    Hi Alejandro, nice work..

    I have a few observations.

    • You don't need to add alt to purely decorative images, and the background images would probably be better if they were a css background image, instead of cluttering your html.

    • Perhaps the h2 would be better in a dl link. The description list element key-value pairs. Something like this ...

    <dl>
<dd>followers</dd>
<dt>80k</dt>
<dd>likes</dd>
<dt>803k</dt>
<dd>photos</dd>
<dt>1.4k</dt>
</dl>

    Happy coding,

    Brendan

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Aisha, here is some answers and advice...

    • 1 - You currently have your img inside the span tag and the h4 outside. It would be better if you used this structure
     <li>
<img src="./assets/images/icon-success.svg" alt="" id="icon">
<p>Product discovery and building what matters</p>
 </li>

    And then wrap all three li tags in a ul tag. Although, even then the image and the text would'nt be on the same line. Because <p> is a block level element. In order to get them on the same line you could declare.

    li {
display: flex;
}

    • 2 - The font size is being applied to the <h1> for me.

    • 3 - I think you could probably find a good tutorial on YouTube for this.

    • 4 - The easiest way to link your success.html would be to take everything inside the success.html <body> tag, wrap it in a <section> tag and place it into your index.html above the closing </body> tag and then toggle display in your javascript.

    -5 - A combination of linear-gradient and box-shadow are used to create this effect. (I['ve linked their mdn pages) and here is useful tool to create box shadows - link.

    • 6 - The span is colored it's just a very dark blue. Try adding a brighter color.

    • 7 - I would suggest completing more newbie challenges on frontend mentor. They let you concentrate on small but important details and after a few you will feel a lot more confident tackling the tougher challenges.

    Hope that helps. Happy coding!!!

    Brendan.

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Vishal,

    It's visible for me on mobile mode.

    It maybe a problem with zoom. When you're looking at in responsive mode try pressing ctrl + 0(On windows). to reset the zoom.

    Here are some observations on your solution.

    HTML

    • The .main container <div>. Should be a <main> landmark element. Landmark elements help to give your page structure.

    • Your li tags don't have closing tags and they all need to be wrapped inside a ul

    • The <h6> tag should be a <label> connected to the <input> like this ---

    <label for=email" />Email address</label>
<input type="email" placeholder="email@company.com" name="email"</input>

    here is the mdn page on labels and how to connect them to an input.

    Happy coding, Brendan

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Rabel, I have a few suggestions for you.

    • The <h3> tag should be a <h1>. Every page needs a <h1> and the heading elements (<h1><h2><h3><h4><h5><h6>) must be used in ascending order. Here is the mdn page on heading elements

    CSS

    • If you remove the margin declarations from .card and the width from main. Then add the following to main
    justify-content: center;
min-height: 100vh

    you will have it all nicely centered using flex-box :)

    • I suggest you look at using relative units instead of pixels. Have a look a rem, em, vh, vw and %. Here is a handy page that converts pixel vales into rem (link)

    Nice solution. Happy Coding

    Brendan.

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Igor, very nice looking solution. A few bits of advice.

    HTML

    • Very little to say here other than I think the change link would make more sense as a <button>. The <a> element is used to link to other webpages while the <button> element is used to trigger a JavaScript action. Here, I assume in fully working app pressing "change" would just change the details of the annual plan.

    • I also don't think you need an alt="" tag on the image. Images that are purely decorative or are described in text nearby don't need alt tags.

    CSS

    • A little margin-block: 2rem on .container would push it away from the top and bottom.

    • I would suggest not using px so much. Try relative units instead %, rem, em, vh, vw. They will help with responsive pages.

    • Also try avoiding setting fixed heights on elements. Use padding and margin instead to reach the desired height. This also helps with responsiveness.

    Well done, look really good.

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Ayrton, nice solution. I have a few suggestions.

    HTML

    • I think you should change the <div class="main"> container to a <main>` landmark element. Landmark elements help to give your page structure. They also help with accessibility.

    • It is also not best practice to have more than one <h1> on a page. - although after reading the headings page on mdn - it says - "While using multiple <h1> elements on one page is allowed by the HTML standard (as long as they are not nested), this is not considered a best practice. A page should generally have a single <h1> element that describes the content of the page (similar to the document's <title> element)."

    CSS

    • First thing I noticed in your CSS is that your put * at the start of every selector. * is the universal selector, which means it selects every element on the page. You only need to use it for quite specific use cases.

    • Generally it's a bad idea to have fixed heights and can create a lot of overflowing issues. Instead try using max-height, min-height.

    • I think rem units would be a better choice for declaring font-sizes. vh and vw could cause some strange font behaviour especially on different screen sizes.

    I hope you find something useful there.

    happy coding.

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi John

    • You should change the <h2> to a <h1>. All pages need a <h1> heading element. The usage notes section on the mdn headings page explains why. Here is the link

    • I would also change the .container div into a <main> element. The <main> HTML element represents the dominant content of the <body> of a document.

    • If you remove all of your media queries and change your .container div width: 90% and max-width: 20rem your site will be responsive. What this is does is let the container grow to a maximum size of 20 rems and if the containing block is less than 20 rems then if fills 90% of the space of the containing block. The containing block here is the <body> element.

    • I would also suggest putting your Css in a separate file. It helps keep everything nice and tidy. Here a tutorial on how to do it if you're unsure

    Good work.

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Randall a nice looking solution.

    I only have a few suggestions.

    • You should change the <h3> to a <h1>. All pages need a <h1> heading element. The usage notes section on the mdn headings page explains why. Here is the link

    • I would also change the .main-qr div into a <main> element. The <main> HTML element represents the dominant content of the <body> of a document.

    • In your css I would suggest you avoid adding fixed width and height declarations and use min-width - max-width instead. This can make your page a lot more flexible to different screen sizes. Same goes for using pixels. It would be better to use relative units like % and rem.

    I hope you find that helpful.

    Good work.

  • Brendan•1,120
    @OneManBanned

    Hi Akshara - A few bits of advice to improve your solution.

    HTML

    • The <h3> should be a <h1>. Remember heading elements must not skip heading levels: always start from <h1>, followed by <h2> and so on.
    • The <div class"main"> should be a <main> tag. The <main> HTML element represents the dominant content of the <body> of a document.
    • You don't need to put each line of text into a <p> tag and a lot of <div> elements aren't required. it would be better to organize if like so ...
    <body>
    <main class="main">
        <img class="type" src="./images/image-qr-code.png">
        <h1> Improve your front-end skills by building projects </h1>
        <p>Scan the QR code to visit Frontend
            Mentor and take your coding skills to
            the next level</p>
    </main>
</body>

    CSS

    • I think if you implement the simplified HTML you may be able to reduce your CSS quite a bit.
    • try not to add fixed heights and widths. Instead use max and min heights this helps make elements a lot more responsive. This also applies to using relative units instead of pixels.
    • For example try removing the padding and margin declarations you currently have on your .main class and instead add width: 90%; max-width: 20rem
    • then center this by adding the following to the .body
    min-height: 100vh;
  display: flex;
  align-items: center;
  justify-content: center;

    and finally this to your <img>

    margin: auto;
  width: 90%;

    You'll notice that adding these more flexible units (vh, % and rem) you have solved most of your problems. Just need to add some margin to the top and bottom and you're home and dry.

    I hope you find some of that useful. Happy coding!!!

  • Brendan•1,120
    @OneManBanned

    Hi Techkie. Your solution looks really nice. Here are some of my observations.

    HTML

    • You need to change the <h3> to a <h1>. Every page needs a h1 tag and the heading tags should be used in order. (so you can't use a h3 unless you have a h2 and a h1)
    • The "Image Button" <img> needs to be inside a <button> element.
    • I would also suggest you change your <div "card"> to a <main> element. The mdn docs on <main> suggest "The <main> HTML element represents the dominant content of the <body>">.

    CSS

    • Set the width of your ".card" container to 90% and underneath it set a max-width: 50rem - you can play with how big you want it. That should stop your container shrinking too fast on mobile layouts.
    • I would also suggest using relative units - such as rems - instead of pixels here is a site that lets you easily convert them.

    I'm not too familiar with using jquery. But your javascript code looks nice and clean and it all works properly.

    Hope you find some of that useful. Well done!!

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Wisslor, well done on a really nice looking solution.

    I have some suggestions for you -

    • You need to add labels to your form elements. This mdn page explains how to implement them and also the additional benefits of using them.
    • You need to add some space at the top and bottom of the page for mobile layouts.
    • I would suggest using relative units instead of pixels this page converts them for you.
    • Have a look at the validityState api. It will help shorten the code you need to write for checking form inputs.

    A really nice looking solution though. Brendan.

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi, the solution looks nice but I have a few suggestions.

    • your project links are all buttons and don't go anywhere. They should be <a></a> that point to the corresponding projects

    • Most of the images in your html would make more sense as background-images in css.

    • I would use <h2></h2> for the section headers - projects, contact - and turn the skills-container into a <dl></dl> .

    • The spans underneath your <h3>'s would be better as <li>'s inside a <ul>

    I hope you find that useful.

    well done, Brendan

    Marked as helpful
  • Brendan•1,120
    @OneManBanned

    Hi Ramesh -

    Your solution looks very nice.

    Although I did notice that if I entered a very long number it overflows the container. setting

    .screen-div { overflow: hidden; }

    would be one way of solving that problem.

    I think that theme toggle would be better as three radio buttons, with the numbers being their labels. That way someone could click on the buttons or the numbers to activate them.

    Also your stylings on the body tag

    body { width: 100vw; height: 100vh; display: grid; place-items: center; }

    On some screens you have a large gap at the top because the contents of the body are being centred. Perhaps using flex-box here and adding some margin to the body would be a possible solution

    Finally, it's very impressive to me that you've done all this with just javascript and css.

    Good work

    Brendan.

  • Brendan•1,120
    @OneManBanned

    Hi Emmanuel,

    If someone doesn't pick a plan level on your solution (arcade, advanced, pro). Your site crashes when clicking onto the confirmation page. You could fix this problem by having one checked by default.

    The rest of your solution looks very nice and works as expected.

  • Brendan•1,120
    @OneManBanned

    Hi Steven,

    if you remove the margin from .right.

    and add

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

    That should centre everything thing in the middle of the page.

  • Brendan•1,120
    @OneManBanned

    I didn't even notice the box shadow on my attempt at this challenge. I'll have to add that.

    Looking at your code for positioning the background image:

               `background-position: bottom 0px right 500px, top 400px left 500px;`

    if you remove "bottom", "right", "top", "left" then you can control the images just using the pixels.

    Try -

                     `background-position: -340px -500px, 700px 300px;`

    Brendan.

  • Brendan•1,120
    @OneManBanned

    Hi, this is the tutorial that helped me with flexbox (https://www.youtube.com/watch?v=3YW65K6LcIA&ab_channel=TraversyMedia).

    Hope it helps.