Skip to content
OverviewSolutions
0
Comments
20
Katie
@kem522

All comments

  • Katie•405
    @kem522Posted over 2 years ago

    Hey Brian - great work! Not much to improve on, super well structured and easy to read.

    My only suggestion would be to reconsider the use of ul and li elements for the followers, likes and photo counts in the card. Semantically, I wouldn't really describe those things as a list of items so would probably expect to see p or span for these instead.

    Keep it up!

    Marked as helpful
    1
  • TsuneWeb•260
    @TSune-webSubmitted over 2 years ago

    Hello, Frontend Mentor community! This is my solution to the Base Apparel challenge✨

    The main image does not seem to resize properly based on screen size. I'd appreciate any advice on working on images, rendering, and positioning, including cropping/ trimming images🖼

    Wishing peace🙏 Happy coding!

    Base Apparel Mobile First

    1
    Katie•405
    @kem522Posted over 2 years ago

    Hi! Really good job - so close to the designs!

    I have a couple of pieces for advice in your CSS (although I can't help 100% with your image issues, they can often be a bit fiddly!):

    • I see you use IDs a lot in order to apply styles, this is generally not considered best practice due to their very high specificity and are best reserved for targeting elements in JavaScript, you can read more about that here

    • I also noticed you added height to a number of your elements. More often than not it is better (and easier for you in the long run) to create height and space by using padding and margins instead of an explicit height.

    Keep up the good work!

    Marked as helpful
    1
  • Katie•405
    @kem522Posted over 2 years ago

    Hi! Good work!

    I noticed that you set each of these padding values separately:

    padding-top: 15px;
padding-left: 15px;
padding-right: 15px;

    This is totally fine and absolutely works but generally I'd expect to see the shorthand version used:

    // where the first value is the top padding
// the second value is the padding for both left and right
// and the last one is for the bottom 
padding: 15px 15px 0;

    You can read more about it here: https://css-tricks.com/almanac/properties/p/padding/

    Happy coding!

    Marked as helpful
    1
  • Katie•405
    @kem522Posted over 2 years ago

    Hello! Great work - well structured CSS and HTML :)

    I see that you used IDs in a couple of places in order to style a component (e.g #desk-text) and just wanted to let you know that due to their specificity this is generally not considered best practise in CSS and IDs should generally be reserved for target an element in JavaScript.

    You can read more about it here: https://csswizardry.com/2012/11/code-smells-in-css/#ids

    Happy coding!

    Marked as helpful
    0
  • Katie•405
    @kem522Posted about 3 years ago

    Hi there, good work!

    I have a couple of pieces of advice for you.

    The first is that for out HTML to be accessible and not repetitive it is better not to have separate markup for the desktop header and the mobile header. Instead it's best to have the content only once and then use CSS and media queries to style it differently at different breakpoints. Here is an example of how to do that: https://jsfiddle.net/kmturley/o45weyvj/

    The other is that you have overloaded some of your elements with classes for example <div class="designed-text-div freeopen-text-div">. In this case all of the rules of the first class are overwritten by the rules in the second class since they both have just width and padding so you should remove the first class from this element.

    Also, for this element <p class="designed-text freeopen-text"> there is no class called freeopen-text so that should be deleted. If you intend to use designed-text here as well then the classname should be more generic as the designed part suggests it belongs to the content higher up the page.

    Hope that was helpful! Happy coding!

    Marked as helpful
    1
  • Eric Nguyen•80
    @EricNguyen1206Submitted about 3 years ago

    This is my calculator application using create-react-app You guys can check my result at domain: https://calculator-react-app-psi.vercel.app/ Feel free to comment if you want to improve anything in this product. Thanks for visiting my repo!

    Calculator application using create-react-app with hook

    1
    Katie•405
    @kem522Posted about 3 years ago

    Heya! Good work! Very nice use of the useState hook.

    I have a couple of pieces of advice mostly surrounding the buttons on the calculator.

    You've used a div for those buttons but making them buttons is semantically preferable as it is more descriptive of what they are and it comes with accessibility benefits, the most important of which is that button elements are keyboard focusable. A user who can only use keyboard can use the tab key to move through interactive elements on the page, divs are not focusable by default but buttons are.

    Another benefit of using the button element is that you can leverage the value attribute in the click events and using e.target.value to pass in the value to the handleNumbers function, would look something like this:

    // the button would now just take handleNumbers as the onClick callback
// I also added a value attribute equal to the value i'd expect this button to have on the calculator

<button onClick={handleNumbers} className={`padButton maths ${theme==='2'?"text-color-2 btn-color-2":theme==='3'?"text-color-3 btn-color-3":""}`} id="seven" value="7">7</button>

// in the handle numbers function you would use e.target.value 
 const handleNumbers = (e) => {
   const number = e.target.value; 

    if(currentVal.length >= 21) {
      alert('Digit Limit Met!');
    } else {
      if(expression === '0') {
        deleteBack();
      }
      display(number);
    }
    setCurrentVal(prev => prev + number);
  }

    In the example above, the only thing that changes between the button numbers is the text in the button and the value attribute, so you could use an array of numbers and a map to create the buttons instead of having to do them one by one:

     {[0, 1, 2, 3, 4, 5, 6, 7, 8, 9].map((num) => {
        return (
          <button
            onClick={handleNumbers}
            className={`padButton maths ${
              theme === "2"
                ? "text-color-2 btn-color-2"
                : theme === "3"
                ? "text-color-3 btn-color-3"
                : ""
            }`}
            key={`button-${num}`}
            value={num}
          >
            {num}
          </button>
        );
      })}

    I was playing around with a very simple version of your app (it just logs the numbers in the console) in this codesandbox to make sure I was giving you the correct advice so feel free to take a look at it as it might be more helpful than trying to read the code here: https://codesandbox.io/s/vibrant-hill-bwbkv?file=/src/App.js

    Happy coding!

    Marked as helpful
    2
  • Katie•405
    @kem522Posted about 3 years ago

    I have nothing constructive to add except that it looks great and you made good use of BEM CSS classes! Keep up the good work 👍

    0
  • Nikolay Nikolaev•20
    @N-NikolaevSubmitted about 3 years ago

    I was wondering when the best time to commit code is. I have very little experience using GIT and I would like to at least know what the general rule of thumb is for add(ing), commit(ing) and push(ing) code.

    HTML component using SASS and Parcel

    1
    Katie•405
    @kem522Posted about 3 years ago

    Hi! Nice work, the styling is very well organised and easy to read.

    In regards to your question about committing ... there isn't really one rule of thumb. I've worked in a couple of different companies and they did things differently and even different teams within my current company have different standards. Sometimes even people within each team commit differently!

    For me, it's important to commit in chunks that are logical so that you can write one short commit message that explains all of the changes in the commit. For example, if I was going to make a calculator app I probably wouldn't commit all of the math functionality at the same time as adding all the HTML and styling.

    This both makes it easier for other developers to review your code by commit and understand what you've done but if something were to go wrong and you wanted to roll back a change you might not have to get rid of all the work. Say my functionality was broken but the styling was fine, if that was all in one commit I'd need to roll back all of those changes instead of just the broken part.

    Marked as helpful
    2
  • Katie•405
    @kem522Posted about 3 years ago

    Looking pretty good! Nice one!

    Some CSS suggestions for you:

    1. You have multiple rule blocks for the same class name in the same media query, namely .container. It's best practice to have these all in the same block:
    .container {
  display: grid;
  grid-template-columns: repeat(3, 1fr);
  height: 500px;
  width: 60%;
  margin: auto;
  position: absolute;
  left: 50%;
  top: 50%;
  transform: translate(-50%, -50%);
  padding: 10px;
}

    1. On line 103 in your CSS you have a flex-direction rule but the value looks like it's for a grid rule. Be careful when switching between display: flex and display: grid! It can get confusing

    2. As web developers we are normally expected to develop UI in a "mobile-first" approach. For CSS the best way to do this is to structure your style sheet with all the base styles being for mobile and then adding media queries for min-widths instead of max-widths:

    // this is my button styling on mobile 
.btn {
  max-width: 100%;
}

// this is my button styling on screens starting at 768px wide (tablets)
@media (min-width: 768px) {
  .btn {
      max-width: 300px;
   }
}

// this is my button styling on screens starting at 1024px wide (desktops)
@media (min-width: 1024px) {
  .btn {
      max-width: 200px; 
   }
}

    Hope that's helpful!

    Marked as helpful
    1
  • adrianna450•25
    @adrianna450Submitted about 3 years ago

    This is my solution for the 3 column card. Any feedback will be helpful

    3 column cards

    2
    Katie•405
    @kem522Posted about 3 years ago

    Great work! The responsiveness works well 😁

    My only suggestion is that generally it is only advised to use one h1 tag per page for both accessibility and SEO purposes!

    You can technically have more, especially if they are in different <section></section> elements but it's generally not best practice.

    You can read more about that here: https://ahrefs.com/blog/h1-tag/#use-one-h1-per-page

    Marked as helpful
    0
  • Kurt Gonzales•300
    @KurtGonzalesSubmitted about 3 years ago

    The grid-gap is not working on my media queries so I adjusted the gap with the use of margins. Any feedback will be appreciated. Thank you everyone. Here is my code: .column-1 { height: 80vh; margin-left: 77px; border-top-left-radius: 10px; border-bottom-left-radius: 10px; }

    .column-2 {
    height: 80vh;
}

.column-3 {
    height: 80vh;
    margin-left: -77px;
    border-top-right-radius: 10px;
    border-bottom-right-radius: 10px;
}

    HTML AND CSS GRID

    3
    Katie•405
    @kem522Posted about 3 years ago

    Nice solution!

    I'm not sure where you were trying to put the grid-gap but I got it working in the browser by adding grid-gap to the .columns class inside the media query. Grid gap should always be applied to the parent element of the grid components, that is to say the same element that you add display: grid; to.

    Hope that helps!

    Marked as helpful
    0
  • Katie•405
    @kem522Posted about 3 years ago

    Good work! The styling is really on point and I love the different color themes 😍

    I see that you're using JavaScript to change the styling of each element but I think you could make it much easier for yourself by using CSS variables.

    Here is an article on CSS variables: https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties

    And here is one about how to use a little bit of JS alongside CSS variables to switch between themes on your site: https://medium.com/@haxzie/dark-and-light-theme-switcher-using-css-variables-and-pure-javascript-zocada-dd0059d72fa2

    In general anything that can be done in HTML or CSS instead of JS should be as the bigger the JS bundle then the slower your site will be. Also, in some cases people may have JS disabled and so you will lose that piece of functionality which might have been able to load otherwise. Finally, you want your JS files to be as simple as possible so they are easier to work on so the more things you can delegate elsewhere the better.

    Hope those articles are helpful!

    Marked as helpful
    0