Skip to content
OverviewSolutions
0
Comments
20

Katie

@kem522London, UK405 points

Web Developer at Spotify 🎧 General Assembly grad 🎓 Feel free to reach out to me on the FEM slack community, especially if my feedback needs clarification!

Latest solutions

No solutions submitted yet.

Latest comments

  • Katie•405
    @kem522

    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
  • Katie•405
    @kem522

    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
  • Katie•405
    @kem522

    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
  • Katie•405
    @kem522

    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
  • Katie•405
    @kem522

    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
  • Katie•405
    @kem522

    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
View more comments