Skip to content
OverviewSolutions
8
Comments
35
John Carruthers
@techyjc

All comments

  • John Carruthers•310
    @techyjc

    Updated the design with the suggested changes.

  • John Carruthers•310
    @techyjc

    Great work! You might want to change the breakpoint as the carousel gets a little small when you resize the browser.

  • John Carruthers•310
    @techyjc

    Updated the carousel and additional code, plus Avatar size at desktop break point.

  • John Carruthers•310
    @techyjc

    Hi Daniel,

    Great work! I did this challenge. The responsive aspect caused me a few problems, it can mean adding in multiple break points because the areas in the design like the 'new' section start looking a bit odd at various screen sizes. Like that you went your own way with the menu hover. I managed to re-create the menu hover underline using pseudo elements (::before) and/or (::After) but it took a little bit of experimentation.

    I initially looked at your CSS and thought if you had used the classes to address the descendant elements directly to reduce specificity but then realised that you were using SASS and were most likely nesting the your CSS selectors.

    Again, great work!.

  • John Carruthers•310
    @techyjc

    Hi Kiran, Great job! Fair bit of CSS for that one.

  • John Carruthers•310
    @techyjc

    Glad you got it fixed. You did a nice job. Is there any particular reason you styled some of the elements locally using the style attribute over using selectors in the style sheet?

    You could add line-height: 1.5; for the text to space the lines more. You could also use font-family and font-size to help match the design. You can get the recommended font from google fonts and copy and paste the link and the css entries from the site.

    Using the <br> element is ok for this but I would recommend avoiding the <br> tag when designing responsive layouts. It's better for the text to flow and break in relation to the container size.

    Hope this helps.

  • John Carruthers•310
    @techyjc

    @AhmedMohamedRefaay I think you put in the wrong link to your published Github page as the preview screenshot is incorrect.

  • John Carruthers•310
    @techyjc

    vh doesn’t take into account address bar in mobile browsers or non floating scroll bars. There is the dvh option dynamic viewport height sizing but it lacks full browser support. The video below covers sizing containers and then use flexbox Justify-content and aligh-items to center the Flexbox content.

    This YT video might help… (https://youtu.be/-sF5KsEo6gM?si=vpMXHedXC4cfjVP7)

    Marked as helpful
  • John Carruthers•310
    @techyjc

    I found the same when I re-created the design. I’m considering redoing the design but creating utility classes for background colors and text colours etc. Then assign the classes as needed. Mainly to see if I can improve on my first attempt.

  • John Carruthers•310
    @techyjc

    Looks great by the way.

  • John Carruthers•310
    @techyjc

    Hi Aleš,

    The default base text size is 16px; so if you're using REM it is based on the Root element.

    You define your root in CSS

    :root { --ff-base-size: 16px; font-size: var(--ff-base-size); }

    Depending on what you are doing em might a better option as it uses the font-size defined in the parent element and scales from there.As you might require different base font-sizes depending on a specific nested set of elements.

    (I think that's right... I'm sure someone will correct me if I'm not...)

    Marked as helpful
  • John Carruthers•310
    @techyjc

    Hi Kiran,

    Hope you're well. Looks great!

    Marked as helpful
  • John Carruthers•310
    @techyjc

    Look like you have a working accordion. Great work!

    It looks like you adjusted the brightness and saturation to achieve the the required result.

    The pseudo element idea would work..

    I took part of your code/css for the cross (.expand ) and edited it to use pseudo elements. I re-created it in Codepen (see the link)- Code Pen - Pseudo Elements

    Hope this helps.

    Marked as helpful
  • John Carruthers•310
    @techyjc

    Great work! From my general observation the semantic markup looks good.

    Adding support for screen readers by adding role and aria-label would be the next logical step. See an example grabbed from the W3c I've included the link at the bottom.

    <section **role="contentinfo"** **aria-label="What kind of Axeman are you?"**> <h2>What kind of Axeman are you?</h2> <p>Are you a Fender guy or a Gibson gal? Well if it’s good enough for Jimi, it’s good enough for me!</p> <p>[…]</p> <h3> The first Fender Guitars</h3> <p>[…]</p> </section>

    W3c Website link

    Marked as helpful
  • John Carruthers•310
    @techyjc

    Great. Just a suggestion use the CSS line-height: 1.5 (or a variant of 1.5) to put space the body text lines match the original.

  • John Carruthers•310
    @techyjc

    Nice work. The structure of the HTML will often reflect the complexity of the design and the connected styling. This also impacts responsiveness.

    If you’re asking in relation to the semantics of your HTML, then there are specific elements you use depending on content. Main is the content container which I don’t think you’ve used. I noticed a section container but I believe this needs a role attribute to be understood by screen readers.

    Then there is the header, footer, article elements which have specific uses. I’ve only just become aware that the H1 tag should only be used once in a page.

    Anyway I’ll stop rambling on..🤣 Again, great work!👏

    Marked as helpful
  • John Carruthers•310
    @techyjc

    Hi Sergio,

    Great work! Just a minor change on your JS. Add a preventDefault to your function.

    element.addEventListener("submit", function (e) => { e.preventDefault; //Do something; });

    Use the submit event rather than click, then you can prevent the default submit behaviour and allow you to process the form information. Add a param e / event to your function to capture events.

    Hope this helps.

    Marked as helpful
  • John Carruthers•310
    @techyjc

    Hi Marcus,

    Thank you, very kind of you. I had made a few adjustments after you commented but before seeing your comment. The email address on the Subscription Success element is a nested empty 'span' with a class. I push the email address entered by the user into the span using JS '.innerHTML'. I completely forgot to style the element at the time.

    I will be updating the email entry form to use regex to validate the correct email address as it's never a good to rely on form elements for proper validation. (Even with HTML 5 feature set). Although probably not as important, as the user could bypass the JS as well.

    John Carruthers.