Skip to content
OverviewSolutions
3
Comments
4
Dorian Alvarado
@Dorian30

All comments

  • Dorian Alvarado•240
    @Dorian30

    I get 404 when I try to access the code, so I'll give you feedback on the design only :)

    • The shadow appears to be missing; the design includes a subtle shadow around the component.
    • The component is slightly larger than the design in both width and height.
    • I am not entirely sure, but it seems the border radius might be slightly smaller too.
    • Good job on the spacing and padding.
    Marked as helpful
  • Dorian Alvarado•240
    @Dorian30

    You can surely guess my surprise when I realised that the found of front end mentor had just reviewed my solution. How awesome and refreshing it is to see that you take the time for that.

    Thanks a lot for the feedback! I totally missed the GitHub link detail and I didn't know you could add those attributes for accessibility, I've been coding for a while but just recently started to pay more attention to this.

    About the media queries I totally agree, I was actually planning to do a refactor of this with an implementation of mobile first! I was kind of used to desktop first 'cause at my workplace we don't often get the mobile design ahead. Just the desktop and we work with that while the design team builds the mobile view.

    Would you say mobile first (min media queries) is the way to go even when you don't have a mobile design ahead (only the desktop version) ?

  • Dorian Alvarado•240
    @Dorian30

    Really interesting Jen! I like this solution. I came here, after you submitted your feedback on my solution. I really liked yours! After reading your feedback I thought I could come and learn some things. From my side I have only a few quick notes.

    I would suggest to use more semantic tags for your HTML. Also, for illustrative images like the wave below the page I think it is better to use pseudo elements ::after or ::before and avoid making your HTML more verbose.

    On a side note, I would like to ask you why choosing rem as your first option. I really like how you kept consistency and only used rem, but why choosing that above the others ?

    Also, I saw that in your HTML you wrote font-size: 67% (or some value near that one, I don't remember), but different browsers can have different font sizes. Here I think we are assuming we will have 16px across every browser, or was this intentional ? In that case, what are the reasons for doing that ? Isn't it better to sent a default font size to reset browsers fonts ?

    Looking forward to your answer!

  • Dorian Alvarado•240
    @Dorian30

    Hi Natasha. I just saw your solution and also answer to you on slack. Really good job for a first project. Keep it going!

    Regarding the feedback, I would tell you to use more semantics tags. Like for example: The top part could be a header

    <header class="header">
  <h1>Join our community</h1>
  <h2>30-day, hassle-free money back guarantee</h2>
  <p>
      Gain access to our full library of tutorials along with expert code reviews. 
      Perfect for any developers who are serious about honing their skills.
  </p>
</header>

    Then, your bottom content could be wrapped with section tag instead and then use an article tag (the article tag is used for whatever part of content that makes sense by itself regardless of page context)

    Also, you can have one h1 tag for every article or section you declare. This helps with semantics and SEO.

    <section class="bottom-content">
        <article class="price">
             <h1>Monthly Subscription</h1> 
              <p class="content-one"><span>$29</span> per month</p>
              <p>Full access for less than $1 a day</p>
              <a class="btn">Sign Up</a>
        </article>
        <article class="us">
          <h1>Why Us</h1>
          <p>  Tutorials by industry experts<br>
            Peer & expert code review<br>
            Coding exercises<br>
            Access to our GitHub repos<br>
            Community forum<br>
            Flashcard decks<br>
            New videos every week</p>
        </article>
</section>

    Lastly I would advice to avoid using br tags. HTML is a markup and it is only for labeling things. If you want to position your elements in a certain always go for CSS instead.

    Also, you can use 2 spaces of indentation instead of 4 (helps readability). About positioning I would suggest to use CSS Grid or flexbox instead of floating elements. It gives more control over your elements.

    I hope this helps! Let me know if you have any more doubts.

    Some reading I recommend:

    https://medium.com/wolox/what-margin-padding-taught-me-after-50-projects-6878b53c903f