Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions9Comments27

All comments

  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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-codes

    Submitted

    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-coffee

    Posted

    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
  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    @GlaDdos

    Submitted

    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

    #sass/scss

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    @NeoAi12

    Submitted

    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-coffee

    Posted

    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

    @lucasbailo

    Submitted

    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-coffee

    Posted

    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

    @JackMorre

    Submitted

    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-coffee

    Posted

    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-C13

    Submitted

    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-coffee

    Posted

    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
  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    @spd237

    Submitted

    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-coffee

    Posted

    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

    @Kamil900215

    Submitted

    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-coffee

    Posted

    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• 900

    @JustANipple

    Submitted

    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

    #styled-components

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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
  • ochijeh Ekene Franklin• 90

    @Ochijehfranklin

    Submitted

    I just finished JS on FreeCodeCamp, and because I love project based learning, I had to try this challenge.

    But for some reason, my JS code does not respond. Kindly help me check it out

    Notification page

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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!

    0
  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    0
  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    1
  • Shha5• 70

    @Shha5

    Submitted

    All feedback is welcome!

    I wonder if the switching images in my html is done correctly. I also had some troubles with the footer - any tips for keeping the footer on the bottom of page and keeping the content centered from all sides will be a great help!

    Thank you!

    Responsive product preview card using flexbox

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    0
  • Vinzz• 280

    @Vinzz34

    Submitted

    My first project with Javascript. I would really appreciate any suggestions for improvement. I learnt Bem Sass and Javascript by building this component.

    Interactive Rating Component

    #sass/scss#bem

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    1
  • Carlos E Alford• 275

    @CarlosEAM

    Submitted

    Hi fellow frontend developers,

    This project was really fun to build. I liked that it wasn't just a calculator, you had to create a theme and also got to use local storage to save the user's theme selection.

    • When using local storage should the users be presented with a screen telling them about the use of cookies?
    • Is there a set of rules when creating a calculator? as in specific use cases to look out for?

    Really appreciate any sort of feedback.. All the best to you all :)

    SASS mixins, HTML, JS

    #sass/scss

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    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

    1
  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    Hi NicholasChristopherBlake!

    I think your solution looks great! I like the fixed navbar on the mobile version, I find it very helpful. One suggestion I would have is too look into transitions in general (to start answering your question). One easy transition to add is for :hover effects which you can easily deal with using this https://developer.mozilla.org/en-US/docs/Web/CSS/transition. About your mobile menu, it seems that you're using the display property to hide/show the menu, but this isn't a property that you can create transitions with easily. You could look into using transform or width, or even left or right if you're using absolute positioning (if I remember correctly). You can look into @keyframes to do more complex animations (probably unnecessary for this drawer menu), it's very fun to use!

    There are many ways to create transitions, and regarding the display property with value of none, it won't be accessible by screen readers last time I read about this. Many people will suggest many things, so I find this SO post helpful with that https://stackoverflow.com/questions/3331353/transitions-on-the-css-display-property .

    Hope this helps!

    Marked as helpful

    1
  • Fanni K• 80

    @fananibanani

    Submitted

    Hello all! This was my first time using SCSS, and while this simple project could've easily been done without it, I am wondering if I could've used it even better for this project? I only used SCSS to make my variables and simple nesting easier.

    Also, I wonder if the accessibility of the page could be improved? I think I did a pretty good job with it, but I am sure I still have a lot to learn.

    And lastly, would a library like Bootstrap or React make the job easier for a web page like this? Or is it not worth using them for such a small project? I am only just getting started learning React, and I'm wondering about the scope of its usage.

    Thanks for any and all feedback!

    Product preview card component solution, using SCSS and Flexbox

    #accessibility#sass/scss

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    Hi Fanni!

    Nice job, it's responsive and your HTML and Scss are nicely organized! Because the project is pretty small, in my opinion you did a good job with Scss. Some other nice features are breaking up your styles to multiple files (and import into other files, called partials, or also modules - slightly different), using mixins can be helpful also. I'm sure you've already been on the website.

    I won't be of much help with accessibility - so hopefully someone else can help with that - I only checked the HTML which looks good for basic accessibility principles, and tried to tab through the page. When I tab the only element I can focus is the "Add to cart" button which I guess it fine since they are no other controls on the page (again, I might be wrong).

    Regarding libraries, you will find various opinions on this, so I'll just share mine and try to justify my thoughts. Bootstrap will probably make this project more complex and the bundle size bigger (unless you use the cdn - but that's making another network request). Considering how much CSS you have, it's probably not worth saving a few lines - if you save any. Since this project has a specific (and simple) design, the only thing I could imagine being helpful is the layout. Which in my opinion still doesn't justify the use for such a small amount of elements. And lastly about Bootstrap and learning (not sure where you are in your journey), most people should probably learn CSS (and Scss and others) and be very comfortable with it before starting to use libraries. In my opinion, what Bootstrap is helpful for, is to make websites that don't already have a design. That way you can have a decent looking page without having to worry about the styling part (once you start using styling libraries you might notice how painful it can be to overwrite their styles - in my opinion they're made to be used as is). About React, just like Bootstrap for this project, it would be unnecessary and wouldn't bring anything else to your project. React is a pretty heavy framework and a great tool, but I believe the use should be justified by this sort of problems: the project needs to make a lot of DOM updates, the project has a lot of pages with a lot of changing parts, ... . Of course you can use any of those frameworks on any project to learn, but maybe aim for one that has multiple pages with repeating components, or one with changing data or lots of user interactions? These might be more suited to learn and understand how helpful these framework can be (once you're there ask yourself "how much work would that be with vanilla Js?", that's what I usually ask myself before using a tool).

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

    Marked as helpful

    0
  • Marcin• 270

    @marcinsuski

    Submitted

    I used grid to keep everything in place in pc view and flex in mobile view. Structure and styling was quite easy to do but I have my JavaScript. I would appreciate any comment on how I could make the js more neat and definitely shorter and more automatic.

    Time tracking dashboard with grid and json

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    Hi Marcin!

    Nice work on this challenge! Your desktop view looks very good!

    Feedback to answer your question(s) about your Js (please note: your solution seems to be working fine, so below is my advice/opinion - it's always a little biased) :

    • you could place your function definitions outside your timeTrack function. What you did is called closure I believe, it's not wrong at all, but it's not needed here (read on it if you're not familiar, it's fun and very helpful in certain situations). You can define your functions outside and call them inside your timeTrack. Going forward, isolating functionality will help you make your code more modular/reusable.
    • your timeTrack function could probably be inside an event listener such as DOMContentLoaded or load, I'll let you read on that.
    • I wonder if you'll be able to use CSS for your 'active' tab instead of adding one class and removing 2 classes? You can style certain states with CSS - like hover - for certain tags that have those states (so you might need to change your HTML tag for this to work - not sure). If you're able to do that, you can make dailyData your event handler. (not tested - just a thought)
    • your dailyData, weekData, ... functions do a lot of work. You should be able to loop through this JSON data without having to do data[0] and repeat your code in each of those data functions to only change one key (those functions are the exact same except for the time key). Look into Js objects and the looping methods associated (Object.keys, Object.entries, ...). There's a lot of good stuff on MDN, and of course google search. Working with data (and objects) is fun and challenging, good job!
    • you can use a linter to help with things like keeping your code in a more logical order. I.e: you have you JSON file in a const but it's in the middle of querySelectors. A linter will probably tell you to move it up or down (if configured correctly). And linters do many more things, they do help most people write cleaner code by following conventions, best practices, ... .

    These are the couple points I observed could be improved outside of your question(using FF):

    • your mobile view isn't responsive all the way to 375px. Around 400px there is no padding around the cards and the cards themselves are being crunched (I get a horizontal scroll bar). The FEM attribution lingers half way on the middle card.
    • on the mobile view, it seems that each card could use a little less padding or bigger font size (not sure of the specs) - if you're doing this with the screenshots (as opposed the design files) that's a good challenge!
    • your HTML file is easy to read and the CSS classes are easy to follow (my opinion), I think you could replace some of those div by other HTML tags if you look at the MDN docs. It's also missing a heading but the report above already says that.

    Hope this helps!

    Marked as helpful

    0
  • Oubaid• 180

    @oubaidelmoudhik

    Submitted

    This project took me almost a week to complete, but that's only because I started wrong, and analyzed the page in the wrong way. After 3 days of coding, I stopped and started from scratch which wasn't the easiest thing to do to just throw away almost 400 lines of cluttered code, but it was the best decision because it gave me the flexibility of writing a cleaner code, and more simple code. This was also my biggest project to this point, and I am so proud of have finished it!

    A question for you: -Was this the cleanest code I could've wrote? -And should I have made the "footer" area 3 divs and include them into the main grid or my way of doing it as a one big div with 3 inner divs the best way?

    Responsive News homepage using Grid

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    Hi Oubaid!

    Very nice work! Your solution is responsive and you made a nice animation for the mobile menu. To answer your question about clean code (and this will be my personal opinion), you'll find that the code you write tomorrow is cleaner than the code you wrote today, and this will go on and on. That's just because you'll keep learning, reading and getting feedback. I think your code is pretty good on this project. You could look into linters (Eslint probably) everyone uses them. You can add them to VScode (maybe other IDEs as well) or to your projects.

    These are the few comments I have about your project:

    • your Js file uses let but you're not re-assigning the variables, you could use const
    • you have 2 unused variables, you can delete them (once you start using a linter, it'll give you this feedback)
    • the way you added the event handlers to your elements isn't wrong, but nowadays people do this all in Js using addEventListener - you should read up on this
    • since you broke up your CSS into files, you might want to look into Scss or similar, they will allow you to break up your CSS without having to include multiple scripts and do as many requests
    • you forgot the cursor: pointer on the mobile menu and a hover effect on its items
    • maybe your nav could be a header, and the actual menu links be inside the nav
    • you're missing an h1, and your headings hierarchy goes from nothing to h3 (unless I missed something). That's not good for accessibility and might impact SEO (not sure about that). Try not to skip a level. Remember that you can style everything with CSS, so think about this from a page structure POV, not from a styling POV.
    • I think the wrapping article isn't what the tag is for, but maybe you could wrap each individual piece/actual article inside an article?
    • your class names one, two, three, ... should probably be more descriptive. In a bigger project you might have collisions or get confused

    About the footer, I'm not seeing the need for a footer here, unless you include the FEM attribution at the very bottom. I think 3 divs inside a div are fine (you should around/read for about this). A good example for a footer is all the landing pages that have menus, copyright, ... .

    Hope this helps, nice work!

    Marked as helpful

    1
  • Daniel Ahn• 60

    @Danny-Devs

    Submitted

    I was curious how to animate transitions when DOM nodes holding reactive data update. I know approaches to animate transitions when elements enter/leave the DOM or are hidden/visible. But animating the tip amount per person and total bill amount per person would be a nice effect, especially compared to the instant change that doesn't provide the sense of satisfaction and wow factor that I'm looking for.

    Tip Calculator App Vue 3 Vitesse

    #vue

    1

    Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    Hi Daniel!

    I wasn't able to access your live solution, the page returns 404. However regarding animating data, I have used Gsap with Vue. It might take a little playing around with it, but it's a nice library. Here are some articles with examples: this logRocket one and this Medium one. You can find more online. Hope this helps!

    Marked as helpful

    0
  • Cats-n-coffee• 670

    @Cats-n-coffee

    Posted

    Hi! Your challenge is responsive from desktop to mobile, it's nice! Once the user clicks to expand sections, the content gets cut off because there is no scroll on the page (I'm on FF). You can probably address this any way you'd like, these are couple solutions that come to mind:

    • add a scroll to your container/page (probably the easiest)
    • limit the height of the card/main element and allow a scroll inside of it (make sure it doesn't overflow the page)
    • allow the user to open one accordion at a time (and close the currently opened one for them)

    There are more ways to address this, I hope this helps! Great job!

    Marked as helpful

    0