Skip to content
OverviewSolutions
9
Comments
27
Cats-n-coffee
@Cats-n-coffee

All comments

  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Mark! Great job on this game challenge! I'll give as much feedback as I can (I'm on FF, 13 inch laptop).

    • the bottom choice button (red choice, fist in normal mode) is hidden by the Advanced and Rules buttons. It could be a layout issue? This results in the red choice being only clickable on its top half, unlike the others which can be clicked entirely.
    • between 700px and 870px the Advanced button overlaps the red choice.
    • when switching to Advanced the bottom 2 options are cut off from the screen, maybe you're missing a vertical scroll?
    • Normal mode in mobile size looks very neat!
    • line 38 of GameChip.tsx could be on multiple lines to make it easier to read.
    • in RoundStarter.tsx, I wonder if you'd be able to make the else if on line 25 easier to read by storing values in a object like this:
    const matches = {
rock: ['scissors', 'lizard'],
paper: ['rock', 'spock'],
// all the rest
}
// then in "else if" you can check the key and values
matches[pickedChip].includes(pickedComputerChip) // this probably doesn't work, but you get the point
    • Your overall project structure is good, and easy to follow!

    Let me know if you have questions!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Nathan!

    Great job for a first DOM manipulation project! I think you could solve your clicked class issue by using <input type="radio" > instead of 5 span elements with class switching. The radio input would give you everything you need for this project with easy way of retrieving the selected value, and easy way to style with CSS (I'm referring to this element, not sure if you're familiar with it, there is also a styling example). I think it would also be better semantics and accessible, since this input is to make the user choose one option only. You can find ways to style these (custom) by googling it, there are a lot of posts about this. I'll give some Js feedback since you said it's your first DOM project:

    • your 4 variables at the top can be const instead of let since you are not re-assigning the variables.
    • to help with your logic as you have it now, you could keep track of the clicked rating in a global variable in your script, and use that number in your submit event handler to update the rating number, that way you don't need to update the DOM before the rating was submitted. Once submitted you can update the rating number element and display the feedback state (so this would be in your submit handler, not in the click handler). The click event would update the rating number (in a variable) and toggle classes. Actually you might want to remove the clicked class on all of them, and add the one class to the selected rating after (remove all, add one). You would not be using toggle in this case, but add and remove class.
    • you can clean your code once your project is finished, remove comments, remove blank lines, ... (only blank lines in your case), it's a good habit to get into if you want to apply for jobs later!

    Great job! Hope this helps, let me know if you have questions!

  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Eduardo!

    Nice job! To answer your question about code size/readability, I think what you have is good! I would suggest moving the "data" from App.js to its own file which you can import to App.js to use (talking about const dadosCardTop and bottom). If the images cause you issues, you have a few ways to get around the problem. You can pass the name (facebook, instagram, ...) to a reconstructed path which you can reconstruct in the src attr (if you use img), you could try passing the full path to image as an object property, or you could make all the icons in a separate component and pass a prop (I had done this here over a year ago).

    Nice job breaking up the components code with Js and Css on their own, this makes things clean and reusable.

    I believe your theme switcher can be simpler. There are different ways to deal with these, my go-to choice is to get CSS variables on the body to match a data attribute. That way you can set all the variables in CSS, you'll reuse the same variable name for both themes but change the values. In your code you can use the CSS variables and once the data attribute changes (using Js) the colors toggle all at once. (example in the same project)

    Great job! Hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Kamil!

    Your solution looks good but I can't see the chart (using FF)! There is an error in the console this.data is undefined which shows the stack trace from generateBars. I'm not sure if it's because this.data is inside an async function and the result needs to be awaited when using it (or run the next step inside the .then()). I wonder if the load event is finished before you get the result of the loadData function since they seem to run in "parallel"? Your code looks clean, would love to see this canvas work!

  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Brodie!

    Nice work! You solution isn't responsive for smaller devices because you hardcoded the width. An easy way to make a card component responsive would be to give it a max-width: 700px (or whatever size/unit you want) and width: 100%. This will help keep your component below a max size while using 100% of the width. For this specific project, you would also need to adjust the layout since the mobile version is aligned as a column with one section below the other. Flexbox might be easier for the purpose of this project, but you can achieve this with Grid as well! Other feedback:

    • You seem to have forgotten the font, I'm not seeing it in the head tag or in your CSS.
    • You can add a hover/focus state to your "Sign up" button and a cursor: pointer, this helps the user seeing where the actions are on the page.
    • Good job picking appropriate tags overall on your HTML!
    • Try to indent your code (HTML in this case), it makes it easier to read and remove unused code such as the main selector in your CSS (it's empty).

    Good job! Hope this helps!

  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Lucas!

    Very nice work! I'll try to answer as many of your questions as I can:

    • 1 I think radio inputs are appropriate for this problem, good choice! One thought: was the div inside the label really necessary? could you add the container__button class to the label or span instead?
    • 2 Have you tried this?
    .redo__button-container:hover .redo__button-text,
.redo__button-container:hover .redo__button {
    color: var(--Orange);
}
    • 3 I'm not sure I understand this one. Might be a few things to unpack here. If you're trying to toggle classes to change from review to thank you page, using display: none is probably going to block you. Maybe this can help? https://stackoverflow.com/questions/3331353/transitions-on-the-css-display-property You can also use @keyframes for more fancy animations/transitions. Those are all CSS transitions.
    • 4 In my opinion, you have too many wrapping divs. Elements can be on their own and have wrappers for layout purposes/design requirements (images, bg, ...). You should be able to have your section and img, h2, p and button directly underneath. Only the radio inputs can have a wrapper to make layout easier.
    • 5 Use let or const but do not use var. Since the arrival of let and const there is no need to use var anymore, it causes headaches and makes your code leaky. You can place all your elements selectors at the top (including checkbox). Try to avoid unnecessary blank lines, and clean up your console logs when you're finished with your project (and commented code, but you don't have any). Great job separating functionality with getRating and changePage.
    • 6 Around 760px wide the component, looks really stretched out, maybe add a max-width earlier?

    Nice work! Hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Jack!

    Great job! Js isn't always easy to use on interactive UIs like this project. Feedback about UI/UX:

    • The error message for the number of people input never goes away. Maybe you can improve this by using a blur event or something similar. Also this same error seems to show when you only have a single digit bill amount, you might need to tweak your logic there.
    • Nice job with the responsiveness! It's only missing something like padding bottom on the mobile version, and the "Custom" tip input isn't readable when the screen is less than 1100px wide for desktops.

    Code feedback:

    • Try to clean up your code when you're finished with your project, in the future if you want to apply for jobs, having a clean codebase will be important. Removing console logs, commented code, ... are all easy fixes that change things in an instant!
    • nice job using inputs! I would suggested replacing h2 with label for "bill" and "number of people", this will improve accessibility and label is better suited for this.
    • Make sure you only have one h1 per page. h1 should be the main heading which is usually a company name or something like that. You can use as many of the other heading levels as you want (h2, h3, ...), but only one h1 per page is better for SEO and accessibility (and HTML semantics). I'm not sure the totals fit well the headings, maybe you can find a more appropriate tag here? https://developer.mozilla.org/en-US/docs/Web/HTML/Element
    • Great job using const, you can actually use it for all the elements you're querying, so you can replace let tipAmount with const tipAmount and same for total and percentCustom. You're never re-assigning the value, so they can be const.
    • I think you could break up your checkEverything function to isolate functionality. The tip calculation can be on its own as well as the people check with the error message.
    • You could place the reset logic inside its own function as well, just to make the code cleaner (though what you did works as well!).
    • I'm not sure why you place the checkEverything inside a setInterval? from my point of view you only need to run checkEverything on user input, so the event listener should be enough, but I might be missing something.
    • Overall, you run the same function checkEverything inside the event listener of each element. If you split this up a little bit more (which would make you code more modular), you can isolate logic to the part that really needs it.
    • line 22 seems like it's missing a space, not sure if that's causing unwanted behavior?

    Nice job, hope this helps, let me know if you have questions!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Vinícius!

    Nice work! That's a very nice solution and handling gracefully those operations. I'll answer your question and give some feedback on what I observed (please note there are other ways to address this theme switcher):

    • I would choose a radio input instead of a range input. To me it would make more sense to provide the user with a "1 choice out of 3" (which is what you did too!) but this is what radio buttons are for. It would be easier to deal with also because you can retrieve the value from the built-in element. With this, you might be able to get rid of the switch and directly assign the value to the body as a class.
    • You could be a little more consistent with the onchange event you added on the html and use addEventListener like you did everywhere else.
    • I think you can take your addEventListener outside each function the wrapping functions don't serve a purpose (such as execOperations, clear, ...).
    • to maintain consistency and standards, pick one type of casing. In certain places you have a blend of camelCase and snake_case (clearAll_button), and in other places you use either or. In frontend the standard is camelCase.
    • Seems like you know how to code - not sure of your background - but depending on the task, vanilla Js (UI projects) are event driven, which mean that functions are probably tied to an event up the chain, and not often ran on their own (like your clear and execOperations do). This calculator is good project to make it only event driven with helper functions like your calcAdd, calcSub, ... . Now this is just feedback for this project, you'll find vanilla Js/Node projects that do not qualify to be mainly event driven.

    Great work! Hope this helps

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Hugo!

    Very nice job! It's responsive, I like sliding effect on the mobile menu and you have nice transitions on hover. Your Js is pretty good, not much you need to change there, maybe a couple things to make it more DRY and clean:

    • in general, remove console logs, comments (as in commented code, not helpful comments of course) and unused code. That's a good habit to get into once you're done working on a section or once your project is finished (it'll be important going forward if you ever want to apply for jobs).
    • there seem to be duplicate lines in your resize and scroll events. You can probably extract this logic in a separate function and call it inside your handlers. If references to elements are an issue because of the scope, declare variables in your global scope and assign the values inside the load event. Those are just elements after all, so placing them in the global scope is fine in my opinion. There are other ways to get around this, but that's one way to address it. You could also pass the elements as arguments maybe?
    • Those same scroll and resize events can probably be taken outside the load event since they are placed on the window as well.
    • setting CSS in Js is fine, another way to do this would be to add or remove a class and place all those CSS rules inside your styles files. That will shorten your Js by a few lines.
    • the load event is fine, I think in your case the DOMContentLoaded event should be enough. Did you try that?

    Great work! Hope this helps

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Toni!

    Nice work! To make your Js less repetitive, you could extract the fetch into a separate function. Once you start working with APIs (if you haven't already), you'd want to make sure that you're not duplicating the requests if it isn't really necessary. Some goes here with your Json file. So I would have the fetch by itself and maybe have the response stored in a global variable (if you use a framework, you'd often store the response in state):

    let response;

async function fetchJson(url) {
  const response = await fetch(url);
  const jsonData = await response.json();
  response = jsonData; // this places the response in your global variable
  
  return jsonData; // the return here might be unecessary since you are placing the response in a variable above, but feel to do this however you want. 
}

fetchJson('data.json');

    The example above keeps things simple, however it's strongly recommended to place any fetch inside a try and catch when using async/await, so you can handle errors (but data.json is local here). Once you have the response stored in your global variable, you can grab it from inside each daily, week, month function. There are many ways to go about this, if you'd like to keep the promises using .then() you can replace async/await with that, you'll be assigning the response to your variable inside a .then().

    I have another way in my solution (pretty close to the above), I didn't use a try/catch because it's just a local file. It's quite lengthy because I'm building the card with Js, but if you're interested: https://github.com/Cats-n-coffee/FEM-timeTrackingDashboard/blob/master/src/index.js

    Hope this help, let me know if you have questions!

  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Kamil!

    Nice work! I'll give as much feedback as I can:

    • the button hover state makes the content jump because you are adding a border, which ads to the box (you're essentially adding pixels around the button). I can think of two easy solutions to get around that: 1- add the border to the button (before the hover), that way the border is already there when hovering and you only need to change the background-color, 2- use outline instead of border on hover since it will add this line inside the button and not outside. Word of caution with this second one, since outline is there by default on focus, it's actually an important state that should be modified carefully/be a distinct state in most cases (in your case you'll modify the color, which might make it the same as hover and that's not a good solution, ... but it's good to know it's possible).
    • your font doesn't seem to have loaded correctly, or you not have it imported anywhere, I can't find it in your HTML or Css.
    • around 1194px width your title is stuck at the very top of the page, which might be because of your layout and the height you're calculating. If you're using flexbox, you should be able maintain all elements within their space by using wrappers (divs) and setting the flex-direction and flex properties.
    • your chevrons (near the menu items) appear broken on my machine (using FF), and the dropdown menus are missing styles.
    • adding an click listener to the document and matching the event target is a valid way, but might not be the simplest way in your case. You could just declare all your elements at the top and place an event listener on each. Since your logic is the same across (adding the active class) you can create a single function for this. If you want to keep track of the current dropdown, take the variable out in the global scope, that way it can accessed everywhere, and you can make a separate function to remove the class. You could also have one event listener per menu item and loop through their child elements to place more event listeners if you'd like. That way you don't need to grab every single submenu item in a separate variable (so you'll do a forEach like you did).
    • in general, try avoiding having callbacks inside of callbacks like you're doing with document.querySelectorAll, Js can easily get you down this "callback hell" and your code will get difficult to deal with.

    Good job! Hope this helps, let me know if your have any questions!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Sam!

    Nice work, images can be tricky. I would suggest a couple changes for this image:

    • avoid setting the container height and width in pixels. Unless specific reasons for setting these with exact pixels, in general you'll want to manipulate the content inside and add padding to the container if necessary. In this case I would set your main width: 100% and max-width to whichever maximum width you think works best. This will allow for better responsiveness on mobile even if the screen gets smaller (in this case). The height should be determined by the content, you might/not need to specify it (in this case).
    • Your image could be more responsive if you have it in an img tag, where it could get width: 100% and height: auto. Again, if necessary you can still have a max-width on it. There is a way to make your images switch for different size viewports explained here: https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images. On a side note, CSS background-image isn't accessible by screen readers because they're not in an HTML element (I might be wrong).

    A couple comments about the code:

    • Make sure you only have 1 h1 element per web page, you seem to have 2. I personally think that the perfume name should be the h1, not the price. If you need other headings you can use as many of the other heading levels as you want (h2, h3, ...) but don't skip a level. It's all better for SEO and accessibility.
    • You have enough styles, place them in a separate file. You can add your styles using .css file extension, and adding them inside a <link rel="stylesheet" href="path-to-your-styles.css"> inside the <head> of your HTML. Here is an article for reference https://www.freecodecamp.org/news/external-css-stylesheets-how-to-link-css-to-html-and-import-into-head/ .

    Hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi!

    Nice job for a first Js project! I'll try to point out things that could cause your script to crash:

    • if you open your devtools you have an error in the console: ReferenceError: notifications is not defined, which means exactly what it says. You'll get used to reading error messages (even if they don't always help it's always a good idea to start there), but this one tells you on line 5 of your script that notifications isn't defined so no value can be read from it. It seems that you forgot to declare your notifications variable before using it.
    • I would also suggest to use a different class to remove on lines 9 and 16 because you are removing the class on the elements you're looping through (looping over the same class). It could potentially create problems (I haven't tried but it's possible) and it also makes it confusing when reading your code. It's okay to add another class to do the read/unread business.
    • You're selecting the elements with notifications class in 3 different places and you don't need to. If you're having issues with the variable not updating, you could keep track of elements inside of an array, and I'll let you think about this and look it up (let me know if you're stuck).
    • your mobile view isn't responsive, the layout looks good until 550px but below that everything gets a scroll.

    Let me know if you have questions, hope this helps!

  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Toshihiko!

    Nice job! You're almost there centering your card. Since you already have the correct flex properties on #wrapper, all you need to do is make this same element take the full width of the screen. You might want to set min-width on your html and body elements as well. I would do 100vw since this card is expected to take the full screen, and your wrapper width: 100%.

    Hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Angelo!

    Nice job! Form validation is never an easy thing, especially when there's an email address in the form (you'll see a lot of people use libraries and validation is usually done in the frontend and backend as well). These are the things that caught my attention:

    • Your submit button is inside a div but the event listener is on the input which is much smaller than your div. At first I thought the button wasn't working because I didn't think of clicking on the text itself. I would suggest removing the div and use only your input (or you could use a <button type="submit"></button>.
    • Your validation is good for basic things such as empty fields, email and password length. You could add a length check for names, and white space trimming for all the fields (except password maybe, I'm not well versed in password validation). Another thing to look into if you're interested is input sanitation, which should be done on both front and backend (most modern frameworks will do this for you).
    • Another fun thing to implement is validation on blur which you'll see used often especially in long forms.

    Js feedback:

    • it seems that you can place most of your selectors at the very top of the file with the ones you already have there (like your errors divs) and they can all be const since you are not re-assigning the values.
    • You should be able to remove the selectors like this one document.forms['myForm']['firstName'].value;, you already have each form input at the top so you can do firstName.value.
    • I think you could place your event listener on the form itself and listen for the submit event, it'll help with code readability.
    • To keep your Js shorter (and slightly improve performance - not that this matters here), you can place a class on each part that changes with validation (error div and input) and add/remove the class in Js. You might be able to integrate the error icon into the class logic as well, so you don't need to loop through them at the beginning and change the background. Your Js isn't as bad as you think :)

    Let me know if you have any questions, hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Shha5!

    Your solution looks good overall, a couple tweaks to the responsiveness like you said might be the only thing missing! I'll give you feedback on everything I see, and don't forget there might be many ways to get to the same results:

    • It's hard to tell if your images are switching, but the end result looks fine. I personally find it easier to reason from a mobile first approach and I tend to use min-width instead of max-width. I also tend to keep the images of srcset and sizes in the same order, so it's easier to understand. Here is another way to do it, not sure if you've tried that https://developer.mozilla.org/en-US/docs/Learn/HTML/Multimedia_and_embedding/Responsive_images#use_modern_image_formats_boldly
    • Between 480 and 600px you're missing some responsiveness and your card starts to shrink and look strange. I usually get around that problem by bringing the desktop version later (once everything fits) and keeping the mobile version until then, which means the mobile version might have some space around it for a while (in certain cases you can expand things a little). Here is an example of what I mean with my recent solution https://fancy-sfogliatella-dec971.netlify.app/.
    • I would try to keep only 1 media query to rule them all, specially in a project like this. Have you tried using only min-width? This is the way I think about things: look at both mobile and desktop version and notice the layout and other differences. This will help you place any extra wrappers if needed. Start by implementing the mobile version and do all changes for tablet/desktop using the min-width. I.e: mobile has display flex row, but when the viewport reaches 600px (min-width: 600px) flex-direction is now column.
    • I like to use display of grid for my general container, that way I can give the main content all the space it needs and the footer can stay at the bottom. There are many ways to do this, I find flex a little tricky for this purpose sometimes, have you tried align-self: flex-end on your footer? Your footer might need to expand (maybe get rid of the flex-shrink?), and if you have a hard time keeping the footer content at the bottom, you can make the footer itself a display: flex and align things at the bottom inside.
    • you're missing a cursor: pointer on the button which I'm sure you forgot.

    Let me know if you have any questions, hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Vinzz!

    Great job for a first time using Js! I think you picked a good project to start with the language. This project is pretty small, so I'll try to give feedback on everything I see:

    • as a general rule, you want to give all your variables a real human readable and meaningful name, so avoid x but maybe use something like button. Maybe the only accepted exception is for loop and the let i = 0 (or similar if you're doing a reverse loop) where i is known to be short for index. Good variable names will help you in bigger projects, and help yourself and others understand your code with the least ambiguity possible. You'll find that good variable names are a real mental exercise :)
    • undefined is a falsey value in Js (among other values), line 18 can probably be if (value) because it'll evaluate to true once it has a value. This is a good page to read https://developer.mozilla.org/en-US/docs/Glossary/Falsy
    • I'll just give my 2 cents on the logic/UX. You're allowing to send a 0 rating, but it's not in the choice list. The user doesn't know that submitting without selecting a number gives a zero rating. They could also accidentally click the submit button without having anything selected. I would suggest to add 0 as an option, or disable the submit button it there is nothing selected.
    • since you said it's your first Js project, I'll point you to another way of looping through elements, where you use querySelectorAll and forEach (this is my recent submission https://github.com/Cats-n-coffee/FEM-timeTrackingDashboard/blob/master/src/index.js#L136). You'll this used often.
    • be careful with innerHTML has it can lead to security vulnerabilities if you are not fully aware of what you're using it for. You seem to be using/retrieving only text and no HTML, so you could use innerText or textContent. There is a difference between the 2 I'm suggesting, but I'll let you read on that.
    • Great job using const and let and thinking through this project!

    Hope this helps!

    Marked as helpful
  • Cats-n-coffee•670
    @Cats-n-coffee

    Hi Carlos!

    Nice solution! And great questions, I'll try to help answering them. I can't see you code on Github because the link seems broken. It seems that you might need to ask for consent to store data on the user's browser storage depending on what kind of data you are trying to store. Accepted answer on this post does a good summary https://law.stackexchange.com/questions/30739/do-the-gdpr-and-cookie-law-regulations-apply-to-localstorage. From what I remember it can also depend on the country we're talking about, the EU (as an example) being more strict than other countries.

    I'm not sure about a set of rules for calculators, the only thing I would suggest is to look into how Javascript deals with certain operations. You might have encountered NaN, which shows up in certains cases (which you can find in the docs). This article does a good job at handling division by zero in Js https://www.educative.io/answers/how-does-javascript-handle-divide-by-zero. I tried doing 0/0, 1/0 and they both return . in your calculator.

    One last thing, from looking at the Js source in the devtools, you could probably use let or const instead of var, because var is in the global scope which can lead to unexpected bugs.

    Hope this help!

    Marked as helpful