Skip to content
OverviewSolutions
3
Comments
5
tomthestrom
@tomthestrom

All comments

  • Vinayak•30
    @d-vinayakSubmitted over 3 years ago

    I am not able to position social icons to the right bottom side for desktop version. I will be really grateful if someone tells me what I am doing wrong plus any additional feedback will be really helpful for me

    Huddle landing page with mobile first approach

    3
    tomthestrom•165
    @tomthestromPosted over 3 years ago

    As @marik999 points out, you could position them horizontally along the main axis using justify content: flex-end; and using some margin or padding, right now you're positioning them to where they are using justify content: center;, that's why they are in the center.

    By the way, speaking about the social media icons, it's cool you decided to put them into a <ul> element, that makes for some nice semantic HTML! But in my opinion it would be better to have each social media icon as a standalone list item (<li>) instead of having just one <li> with 3 child elements, that basically defeats the semantic approach introduced by using ul and li for these social media icons.

    Take care, Tom

    1
  • drallas•375
    @DrallasSubmitted over 3 years ago

    This is my 3rd newbie challenge submit and I enjoyed the process of hacking this card component together. Building such a 'simple' card component isn't easy for a 'Newbie' and it requires some time to think over before trying to do it properly.

    Feedback would be welcome How is the responsive experience; what can / should I improve (*see Note)? I try to use BEM but still have doubt if i used it correctly Anything else that I missed or should improve.

    Note: Related to BEM I recommend this video: https://www.youtube.com/watch?v=iyR6RXUZFQ8 it's quite good and explains it in visual detail.

    Where I struggle is how should elements be named when there are multiple levels of nesting.

    <div class="card__mid">
          <div class="card__text">
            <div class="card__name">Victor Crest <span> 26</span></div>
            <div class="card__location">London</div>
          </div>
        </div>

    The Profile Card Component (HTML & CSS only)

    4
    tomthestrom•165
    @tomthestromPosted over 3 years ago

    Hi Drallas,

    here is how you could do it with linear-gradient (and background layering):

    1. remove the element with class="card-top"
    2. apply this to the element with class="card": background: linear-gradient(to bottom, rgba(0, 0, 0, 0) 0 39%, white 39% 100%), url(./images/bg-pattern-card.svg);

    So basically you are:

    1. setting a gradient from top to bottom
    2. setting a background image on the element
    3. applying an opaque color from 0-39%
    4. the background image is visible in the 0 - 39% horizontal range through the opaque color
    5. the background image is covered with white in the 39 - 100% range.

    Let me know if it's not clear.

    Have a good day,

    Tom

    1
  • drallas•375
    @DrallasSubmitted over 3 years ago

    This is my 3rd newbie challenge submit and I enjoyed the process of hacking this card component together. Building such a 'simple' card component isn't easy for a 'Newbie' and it requires some time to think over before trying to do it properly.

    Feedback would be welcome How is the responsive experience; what can / should I improve (*see Note)? I try to use BEM but still have doubt if i used it correctly Anything else that I missed or should improve.

    Note: Related to BEM I recommend this video: https://www.youtube.com/watch?v=iyR6RXUZFQ8 it's quite good and explains it in visual detail.

    Where I struggle is how should elements be named when there are multiple levels of nesting.

    <div class="card__mid">
          <div class="card__text">
            <div class="card__name">Victor Crest <span> 26</span></div>
            <div class="card__location">London</div>
          </div>
        </div>

    The Profile Card Component (HTML & CSS only)

    4
    tomthestrom•165
    @tomthestromPosted over 3 years ago

    Hey, I think you're using BEM good, but since I already checked out the code a bit, I have something to add.

    Creating the element with class="card-top" could be avoided by using linear gradient for the card background. That element (card-top) has no semantic value as it is now.

    Alternatively in this case if you're styling and creating an element only for the purpose of implementing a design, try doing it with the before and after pseudo-elements.

    margin-right: -50%; on the card is pointless, you're already positioning it via position and transform.

    Good luck and have a good day,

    Tom

    1
  • tomthestrom•165
    @tomthestromPosted over 3 years ago

    Hey Nicole,

    I was looking at your JS code. It's nice and readable, just a few things that are mostly a matter of taste.

    This whole code could be wrapped in an IIFE to avoid polluting the global namespace. It's a basic app and does no harm doing it the way you do, but it's a good practice to think if you really need to have globals before using them (build good habits from the beginning).

    let monthlyPrice and let annualPrice should be const, they are not reassigned anywhere.

    const toggle = document.querySelector("#toggle"); could be a getElementById, it tells better what you're doing since querySelector is more of a general tool.

    querySelectorAll based on class names are not the best, it's a better practice to use data attributes, again at this scale of an app it doesn't matter that much, just something to be aware of.

    toggle.addEventListener("click", function () {
  if (toggle.checked) {
    for (i = 0; i < annualPrice.length; i++) {
      annualPrice[i].style.display = "none";
    }
    for (i = 0; i < monthlyPrice.length; i++) {
      monthlyPrice[i].style.display = "block";
    }
  } else {
    for (i = 0; i < annualPrice.length; i++) {
      annualPrice[i].style.display = "block";
    }
    for (i = 0; i < monthlyPrice.length; i++) {
      monthlyPrice[i].style.display = "none";
    }
  }
});

    This could be shortened for readability and to reduce repetitiveness to:

    toggle.addEventListener("click", function () {
    for (i = 0; i < annualPrice.length; i++) {
      annualPrice[i].style.display = this.checked ? "none" : "block";
    }
    for (i = 0; i < monthlyPrice.length; i++) {
      monthlyPrice[i].style.display = this.checked ? "block" : "none";
    }
});

    You could also use Array.map() or Array.forEach() instead of those for loops to make it more declarative.

    Your github repo shouldn't include the .DS_Store file and the package-lock.json.

    Have a good day,

    Tom

    0
  • tomthestrom•165
    @tomthestromPosted over 3 years ago

    Hey Eric, it's easy to read, I see what it means to do, good job on that. Since you asked for feedback, I have a few things to say.

    Did you test it? You're using a validEmail() function that is not defined.

    Everything is in global namespace, which for this app with a few lines is alright, but you could solve it with an IIFE.

    isvalidateEmail is not camelCase and sounds like it does 2 things, while it only does one. I would call it isValidEmail since it returns true or false based on a regex.

    You could extract the part where you do:

          email.classList.add('error');
      email.innerText = 'Email cannot be blank';

      email.classList.add('error');
      email.innerText = 'Email is invalid!';

    into a function. You could have a place to store these error messages, and then just pass the error message to email.innerText based on the given parameter.

    const small = form.querySelector('small'); this way of selecting an element is error prone, you should either use an id or a data attribute. You need to be able to always uniquely identify the element you wanna manipulate. Imagine you'd have another <small> element in the form, this code could break.

    Also for the github repo, you should not be having a directory and nothing else at the root level. You should move the contents of that directory into root. While at it, I would include a .gitignore and a README.md file.

    Have a good day,

    Tom

    0