All comments

Mark•210
@MarksKolbanevsSubmitted over 1 year ago
Any feedback is welcome!
Rock, paper, scissors game on React.tsx with scss
1
Cats-n-coffee•670
@Cats-n-coffeePosted over 1 year ago
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
1
Nathan Turkson•120
@nathan-codesSubmitted almost 2 years ago
I just learnt how to work with the DOM so this challenge was perfect.

I got 90% of the challenge working. I just had a trouble removing the ".clicked" class each from previous buttons when a new rating button is clicked.

I would appreciate some help solving this.

Any additional feedback, comments or advise on best practices and code refactoring would be appreciated as well.
Using the JS DOM to solve the Interactive Rating Component Challenge
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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!
0
Edüardo•370
@MendesEduardoSubmitted almost 2 years ago
How to reduce code size and make it more readable?
Social Media Dashboard in React.js
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
0
kamil kawa•540
@GlaDdosSubmitted almost 2 years ago
Chart is done using canvas, was harder to do than i expected, but it is working, even though it is a bit messy.
Expenses chart component
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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!
1
Brodie•110
@NeoAi12Submitted almost 2 years ago
So this project felt a little more natural. Please check after me for errors, duplications, and excessiveness. Also when I look at site on mobile devices it doesn't shrink down, you have to scroll horizontally to view entire card. Please help and thank you!!
Single Price Card
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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!
0
Lucas Bailo•100
@lucasbailoSubmitted almost 2 years ago
Hello guys!

This project was the hardest and longest project that I've ever done ultil now.

I took the liberty of creating a button to the users change their rating, if they want to.

Some problems that I had to figure it out:

1º - The rating buttons are not really buttons. They are an input with "none" display inside a container shaped as button. This way I was able to "hold" the clicked rating in the button! If you have a better solution, tell me please, because it took me more than 5 hours to discover it!
```
<div>
   <input type="radio" name="rating" id="button1" class="checkbox__class" value="1">
       <label for="button1">
          <div class="container__button"><span>1</span></div>
  </label>
</div>
```
2º - The second problem was to create only 1 hover to change the style of 2 classes. But I didn't manage to do it, so I created 2 codes with one hover for each part:
```
.redo__button-container:hover .redo__button-text {
    color: var(--Orange);
}

.redo__button-container:hover .redo__button {
    color: var(--Orange);
}
```
3° - How to put a transition time when the JavaScript change the styles of the pages?
```
/* Main page state - here the JS will change the display to none when SUBMIT button is clicked*/
.rating__state {
    display: block;
    transition: 1s; /* Didn't work, I'll try to fix it with toogle */
}

/* Thank you page state - here the JS will change the display to flex when SUBMIT button is clicked */
.thankyou__state {
    display: none;
    flex-direction: column;
    justify-content: center;
    text-align: center;
    transition: all 1.5s ease;
}
```
4º - I used a lot of tags to create the HTML, but I'm not sure if all of them are being used correctly. So, feel free to correct me in all of them!

5º - The JS code! As I'm a beginner, I don't know if I'm using the best paths to get the variables and values. So, if there's a better way to do it, let me know!
```
var buttons_container = document.getElementsByName("buttons_container")
var submit = document.querySelector("[data-button]")
var place_rating = document.querySelector("[data-rating]")
var mainPage = document.querySelector("[data-mainPage]")
var thanksPage = document.querySelector("[data-thanksPage]")
var redo = document.querySelector("[data-redo]")

submit.addEventListener('click', () => {
    getRating ()
    changePage ()
})


function getRating () {
    let checkbox = document.querySelector('input[name="rating"]:checked');
    let rating = checkbox.value
    console.log(rating)
    place_rating.innerText = rating
}

function changePage () {
    mainPage.style.display = "none"
    thanksPage.style.display = "block"

}

redo.addEventListener('click', () => {
    mainPage.style.display = "block"
    thanksPage.style.display = "none"
})
```
Testing my HTML, CSS anb JavaScript skills!
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
1
Jack•120
@JackMorreSubmitted almost 2 years ago
The JS was a difficult for this one. I had a whole file and spent two days trying to figure out what I was doing wrong. I was trying to be clever and use when ever someone clicked is would refresh but this caused more issues that I would have thought. So i took a different approuch which seems to work and now when you click or fill in a input, it will automaticall update.

Let me know what you think.
Tip calculator app main with JS
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
0
Vinícius Costa•120
@Vinicius-C13Submitted almost 2 years ago
the most challenging part of this was to create the theme selector. I decided to do this using a range input with an onchange event, which triggers a function with a switch case, taking the range input value into account to decide what theme should display.
CSS variables, CSS grid, Javascript
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
0
Hugo Delbegue•120
@HUGODELBEGUESubmitted almost 2 years ago
Hello, for the Javascript part, to get the same result, is there a better way to write the code ?
Responsive homepage with Flexbox
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
1
Toni•420
@ToenShSubmitted almost 2 years ago
This was my first time working with a json file to pull data from so I learned a lot on that. I had a problem with my JS though where I would like to know if there's a way I could refactor my solution into one single function since there's very little that changes between the three functions I already have. That and any other feedback on the design and functionality is much appreciated! Thanks!
Time Tracking Dashboard
2
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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!
0
Kamil Matyszczak•40
@Kamil900215Submitted almost 2 years ago
Hello,

I have problem with black "learn more" button on hover state it adds something that makes top text bouce up. I am not sure for JS code... I think it can be more simplier. Can you please give me any feedback?

Kamil ʕ•́ᴥ•̀ʔっ♡
Responsive intro section with dropdowns
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
0
Sam•970
@JustANippleSubmitted almost 2 years ago
Which areas of your code are you unsure of? the image had spaces on the right and on the left, so i adjusted the width and height of the box to mantain its aspect ratio. This didn't feel like the right thing to do. Maybe there's a better way to mantain ratios but filling the box at the same time
Responsive product preview using aspect ratio and flexbox
1
Cats-n-coffee•670
@Cats-n-coffeePosted almost 2 years ago
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
0

Cats-n-coffee

All comments

Rock, paper, scissors game on React.tsx with scss

Using the JS DOM to solve the Interactive Rating Component Challenge

Social Media Dashboard in React.js

Expenses chart component

Single Price Card

Testing my HTML, CSS anb JavaScript skills!

Tip calculator app main with JS

CSS variables, CSS grid, Javascript

Responsive homepage with Flexbox

Time Tracking Dashboard

Responsive intro section with dropdowns

Responsive product preview using aspect ratio and flexbox