Skip to content
OverviewSolutions
22
Comments
103
Arturo Simon
@artimys

All comments

  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Well done Harold, looks good and is responsive.

    Design Feedback:

    • the styling for your btn-trial is better applied to your anchor tag inside. If you need to make the anchor tag behave like a div. Add display: block to your link. Currently the only section that is clickable is the text. If clicking on the purple background nothing happens.
    • add a focus style to your form submit. At the moment I can't see any visual cue when I tab to it.
    • the "Terms and Services" usually would redirect a user to a separate page. Anchor tag with a hover state is recommended

    Javascript Feedback:

    • There's two classes you toggle between in your form but I don't see any styling for .valid and .invalid, could be removed.

    • Add an event listener using the "submit" event instead of the onclick=validate. So when the form fails validation you can stop the form from submitting.

    form.addEventListender("submit", validateForm);

function validateForm(event) {
    event.preventDefault();
}
    • For the error styling of the text boxes. I suggest wrapping "each field with their error span" into one div. Example below then when "First name" becomes invalid you can add the .validate class to the div which takes care of the error styling like the border color.
    .validate {
   input {
      borderColor: "red";
   }
}
<div id="some-identifier-for-first-name" class="validate">
    <input type="text" id="fname" name="fname" placeholder="First Name">
    <span id="error-fname"></span>
</div>
<div id="some-identifier-for-last-name">
    <input type="text" name="lname" id="lname" placeholder="Last Name">
    <span id="error-lname"></span>
</div>
    • I would challenge yourself to make a new function that accepts two arguments, the id of the field error div and an error message. That way you consolidate calling activating the error styling and messages.

    Overall great job 👍👍

    0
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Nice work Blessing Peter. I'll leave some feedback below. Hope it helps.

    • The 2 background images are tricky. An easier solution I highly suggest is using multiple background images. Link here for more info

    To improve on your current background solution because they are position: relative they are part of the flow of elements and is possible that it makes your content wider than the viewport (browser screen). You can try position: fixed and play with positioning using left, right, top, and bottom.

    • The <font> tag is old and obsolete. Instead use <span> as a replacement. For this tag I would recommend the <small> tag.

    Keep on coding!! 👍

    0
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Nice work Harold 👍 the site responds well. I have a few suggestions.

    Design feeback

    • The "Try it Free" and "Get Started For Free" button could use hover styles
    • The 3 middle sections could be centered on desktop layout. Add a max-width and margin: 0 auto to center it

    HTML feedback

    • Huddle logo - add a more descriptive alt text like "Huddle Logo"
    • For footer phone anchor tag, use tel: in the href tag. Example: <a href="tel:15431234567">+1-543-123-4567</a>
    • For footer email anchor tag, use mailto: in the href tag. Example: <a href="mailto:[email protected]">[email protected]</a>
    • Try the <input type="email"> for your input email address
    • I recommend making the footer social icons with anchor tags so it can utilize the tab key

    Keep on coding!! 👍

    0
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Great job with the challenge Esteban 👍👍. You captured the testimonial card details really well.

    The only feedback I will provide is to organize your css classes to remove duplicating styles. For classes div.testimonial__grid1 through div.testimonial__grid4, they share a lot of common styles that make up the card.

    I'll leave a simple example on how to approach it below. You can change the names to your liking. I'm just naming them using the BEM way. More info on BEM if interested

    .testimonial-card {
    display: flex;
    flex-direction: column;
    padding: 30px;
    border-radius: 10px;
    box-shadow: 10px 10px 110px -51px rgba(0, 0, 0, 0.75);
}
.testimonial-card--grid1 { 
// add specific styles that make grid1
}
.testimonial-card--grid2 { 
// add specific styles that make grid2
}

<div class="testimonial-card testimonial-card--grid1">
    ....
</div>
<div class="testimonial-card testimonial-card--grid2">
    ....
</div>

    Great work!! Keep on coding 👍

    1
  • Rayane•40
    @RaayaneGomes97Submitted almost 4 years ago

    Hi, I have a question. When I finished the project, I realized that I forgot to use bg-mobile. Consequently, bg-desktop was used on mobile devices. Would that cause problems with loading or something?

    Huddle landing page with a single

    2
    Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Nice job Rayane, 👍 the mobile and desktop layout looks great. The typography looks great

    To answer your question about using the wrong image version on mobile. Depending on the size of the image it could greatly affect the user experience who don't have the best cell network connection(slow 3G) or have a limited data plan for their phones. It creates a longer wait time for your site to load and could make a user impatient and move on. For most who have fast cell speeds and on desktop computers will most likely not feel it. Which is why we should aim to make our sites as optimal as we can and always consider users with slower speeds

    Design feedback

    • I tried tabbing through your links and button and noticed the outline: none was removed from your button. For accessibility purposes it's actually a crucial thing to not remove any styling that that can help identify a focus state of interactive controls. If you do remove it then it's highly advise to customize the styling so it at least fits with your design.
    • Look into the resizing the desktop illustration when the design switches to desktop layout. It creates a horizontal scrollbar for the viewport.
    • Hover styling for the social icons would be helpful.

    Keep on coding!! 👍

    1
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Nice job job Harold, looks good and is responsive. 👍

    I noticed something is generating a horizontal scrollbar on your page. I think width: 100vw on your .banner-container is causing it.

    Observations:

    • You're missing some <li> tags for the footer social icons and also wrap the icons with an anchor tag.
    • Lighten the box-shadow color a bit so it's not so intense
    • For the footer phone number and email. Wrap them with an anchor tag and maybe add the prefix in the href to let the browser know the links are an email and phone number
    <a href="tel:+1-543-123-4567">
    <img />
    +1-543-123-4567
</a>
<a href="mailto:[email protected]">
    <img />
    [email protected]
</a>
    • Add a max-width to your desktop layout main container.

    On mobile I think the main container can use a little little bit more left and right padding so it's not too close to the viewport sides.

    Keep it up 👍👍

    0
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Hello again Felipe, amazing job on the challenge.👍👍 It functions and responds well. As usual feedback below.

    Accessibility

    When I enabled some tablets, I tried tabbing through them and was unable to do so. Mainly due to the.table tablet--remove-icon and .tablet elements being divs. I would suggest using a button or anchor tag. By default they would have keyboard support for spacebar/enter keydown and tabbing.

    Design

    • The h1.Job__position could use an anchor tag with a hover state.
    • Minor changes like adding a font-weight to your tablets and maybe decrease font size for your Job__meta

    Great job 👍, can't wait to see your next solution

    0
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Hi there Bouhaouliane, You did a great job👍👍. It's responsive and were able to match the design pretty well.

    • I suggest adding focus state styles to your toggle and 3 card buttons. As I tab through I can't see where I'm tabbing through.

    • The top/bottom corners of the white cards that face the purple card are currently rounded. Remove the radius on just those 4 corners. border-top-left-radius: 0; as an example

    Happy coding!! 👍

    0
  • Arturo Simon•1,785
    @artimysPosted almost 4 years ago

    Good work on completing the challenge ChrisPrzR 👍. I'll leave my feedback below.

    For the icon in the input field. Note that it's not possible to add an image to an input's pseudo selector (:before, :after). Background-image yes but you're using font-awesome and I recommend adding a pseudo selector on the parent container of your input. message-input. See below for font-awesome in pseudo selector

    .message-input:after {
   position: absolute;
   font-family: "Font Awesome 5 Free";
   font-weight: 900; // Important otherwise it won't show
   content: "\f1d8";  // find unicode on font-awesome website
   // play with position
}

    Design:

    • I believe I went with a really small font for the phone illustration. It's weird because too small is not legible but needed to match the design. Here's my solution if you'd like to compare.

    • Look into why there's a horizontal scrollbar. Actually for both .figure1 and .figure2, use position: fixed instead of absolute. They do look a bit squeezed

    • The heading and paragraph font size could be larger.

    Also check out your solution report to fix key things in your HTML markup. Keep it up 👍

    1
  • Arturo Simon•1,785
    @artimysPosted about 4 years ago

    Really nice work Kelvin 👍👍 especially with the animations. I love the animation on the intro section especially when resizing the browser

    Just a few observations. In the mobile menu when hovering over the text it blends in with the background. When the mobile menu is open. It's possible to scroll down and see the remaining page design.

    Just my opinion for the animations after the intro section, section.about-company. To have it only run once when scrolling down. If I scroll back up and back down the content should remain static after.

    Great job and keep on coding 👍

    0
  • Arturo Simon•1,785
    @artimysPosted about 4 years ago

    Really nice work Dusan 👍👍 It responds really well and I like the medium layout after the mobile layout. My only feedback is to add a max-width to your .main-container in the desktop layout so it doesn't expand in width too much.

    Keep on coding!!

    1
  • Diego-Mike•85
    @Diego-MikeSubmitted about 4 years ago

    Hi, this is the firts exercise i did in this website, i didn't do it as good as i wanted to, but, i tried my best !

    If someone can give me and advise o help me out with something, i would be happy for it !

    HTML CSS (Grid)

    1
    Arturo Simon•1,785
    @artimysPosted about 4 years ago

    It's a well attempt Diego, I'll provide some thoughts below.

    • I see a lot of duplicate styling to your .fundamental-n classes. I'll refer to them as a card from here. Before writing any css, look at the design and see if you notice similar patterns of styles first. You can then create a base class that share those common styles then create another class to differentiate it. Example below. At this point your .card is a component that can be re-used multiple times. I'm using BEM as a way of naming components. If interested more info here
    <div class="card card--purple">
        <img class="card__image" src="images/image-daniel.jpg" alt="Daniel">
         ...
</div>
<div class="card card--gray">
        <img class="card__image" src="images/image-jonathan.jpg" alt="Jonathan">
         ...
</div>
    • For your CSS grid. I can't see a reason for the media query on your fundamental. But a few tips to help with the grid.

    Remove the min-height, grid-template-rows from your .fundamental. Also remove the align-items: center which should not squeeze the card's height and doesn't benefit because the cards do not need to be vertically centered.

    • To help with not calling the same values over and over again. I recommend looking to CSS custom variables. SCSS can be also useful for variables plus other things to making writing CSS easier, especially when writing out BEM.

    • I recommend adding a max-width using a px based value to your main container .fundamental. There's a point where the viewport can get really wide and could possibly skew the design. It highly depends on the design but in this case I think it's needed.

    My apologies for it being rather long. Keep it up. It's all about practice 👍👍

    0