Skip to content
OverviewSolutions
6
Comments
26
Sean Red
@seanred360

All comments

  • Sean Red•635
    @seanred360

    What do you specifically mean by " add active states" which element needs it and what does it look like? I can help you if I understand it better.

    ignore this

    This looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short commentThis looks like a short comment

  • Sean Red•635
    @seanred360

    I added scrolling to the screen part of your calculator app. You can see my changes here https://github.com/mvelardeq/calculator-app/pulls and merge the changes if you wish.

    You need to add these CSS rules to achieve the desired effect.

    .screenSection {
  width: 100%;
  height: 117px;
  display: flex;
  align-items: center;
  justify-content: end;
  padding: 1rem;
  border-radius: 0.4rem;
  font-size: 1.7rem;
  overflow-y: scroll;
  overflow-x: hidden;
}

.screenSection p {
  width: 100%;
  height: 50px;
  word-wrap: break-word;
}

    The important part is we want the <p> tag to have word-wrap: break-word so that the number will break without using spaces between numbers. We also need to set a fixed height on <p> and set the overflow-y: scroll on the .screenSSection. This means that the numbers are fixed at 50px and when it gets bigger than 50px we want to have a scroll bar.

    Marked as helpful
  • Sean Red•635
    @seanred360

    I have added a pagination component to your site. See my pull request here https://github.com/Felistus/knowCountries/pulls.

    The logic we want to implement for the page selector component is: There are 8 page links shown at any time (e.g. 1 2 3 4 5 6 7 8 ) unless there are less than 8 total pages. The active link (current page) is in the 3rd position, except for when the active link is less than 3 from the last position.

    To split the countries into pages we need to do this to our array of countries function paginate(items, pageNumber, pageSize) { const startIndex = (pageNumber - 1) * pageSize; return _(items).slice(startIndex).take(pageSize).value(); } Note I am using the Lodash library to shorten the code. This will take the correct slice of countries at the correct position from our array and give us the current page of countries. Then instead of passing the entire countries array into the <Countries/> component, we pass just one page. When we change pages we get a new slice of 8 countries and pass it to the <Countries/> component and the React will re render.

    Marked as helpful
  • Sean Red•635
    @seanred360

    I fixed the 'border-radius only on top of bars' issue. You needed to add a borderSkipped property to the dataSets object. The default is true, but you need it to be set to false. Look at the example here https://www.chartjs.org/docs/3.4.1/samples/bar/border-radius.html. I made a pull request on your GitHub here https://github.com/ABaker14791/Expenses-Chart-FE-Mentor/pull/1. You can view the changes and merge it to your code if you wish.

    Marked as helpful
  • Sean Red•635
    @seanred360

    Can you explain what you mean by horizontal overflow? I do not see any horizontal overflow issue. This solution looks great, I do not see any problems with responsiveness. Make sure that you read the HTML validation and accessibility report. It seems you have 18 accessibility issues and 1 HTML validation issue.

  • Sean Red•635
    @seanred360

    I have fixed the issue with your dropdown menu. Please check my pull request on GitHub to merge the changes. The problem was you have deprecated jQuery code, you need to use the new syntax or it won't work correctly. You can go to the GitHub page and click on the Issues tab, next to <> Code and Pull Requests to see the bugs I have found and how to fix them.

    Marked as helpful
  • Sean Red•635
    @seanred360

    You should use a radio button for the theme toggler. This is more semantically correct and it lets keyboard users and screen readers use your toggler. You should refactor your code to have 1 theme toggler function that accepts a theme object as an argument. You need 3 theme objects. The theme1 object should look like this const theme1 = { body: "hsl( var(--clr-main-bg-blue))", text: "hsl( var(--clr-text-white))", keyButtonText: "hsl( var(--clr-text-grayishBlue))", screenBg: "hsl( var(--clr-screen-bg-blue))", keyButtonBg: "hsl( var(--clr-screen-bg-blue))", };

    that way your code is shorter and you only need to make changes in 1 place. I made a pull request on your Github. You can read my comments and see how I would implement these changes

    Marked as helpful
  • Sean Red•635
    @seanred360

    The useEffect error is a linting error. A linting error means your code will run but you used very bad practices. I think if you understood useEffect you would know how to fix it, let me explain. useEffect is a function that has 3 use cases. Case 1: useEffect(()=>{ doSomething() }) this will run on first render and then re run every time the component renders again. Case 2: useEffect(()=>{ doSomething() },[playerTurn, playerMark, gameActive, gameType]) this has a dependency array at the end of all the variables you used. useEffect will run once on first render and then run again every time one of these variables change. Case 3: useEffect(()=>{ doSomething() }, []) the dependency array is empty here. useEffect will run once on first render and never again.

    The linting error is telling you to either remove the dependency array, or move all your variables into the dependency array. Currently you have it set to only re run when playerTurn changes, but useEffect does not know it also needs to change when playerMark, gameActive, gameType change because you used them. If you put those variables into the dependency array, you app will infinitely re render and crash.

    To fix your problem just remove the dependency array so that useEffect re runs on every render.

    Look at my pull request on Github to see my fixes.

    Marked as helpful
  • Sean Red•635
    @seanred360

    Thanks for posting this! This is a great chance for us to practice Redux. I really like the design of the login page, it is very minimalistic and clean. I recommend writing on the page how to login. I almost clicked away because I wasn't sure what I was looking at. I notice on Redux you are dispatching the UPDATE_LOGIN_FORM_INPUT action every time the user clicks on the pin number pad. I think this is a bad practice, because you are essentially submitting an incomplete form every time. In this situation I would keep the current input in the components local state. You should call the UPDATE_LOGIN_FORM_INPUT when you submit the form (in your case this is when the number of digits is 6). You could also implement a wrong pin number state. I would add a login error state so the user knows they made a mistake. You can put this in the payload of the action.

    I made a pull request on Github if you want to see my suggestions.

    Marked as helpful
  • Sean Red•635
    @seanred360

    To answer your question: I tried loading the page with a simulated slow 3g connection and the page loaded normally. I would say it is normal for a page to load different parts at different times. If the first paint loads in under 4 seconds, I would say there is no problem with your app. Modern apps use lazy loading. Which means it doesn't load elements that are not visible to the user yet. Tiktok, Facebook, Youtube all load more elements when the user scrolls. This ensures the pages always load quickly and do not waste data on things the user may not even see.

    Marked as helpful
  • Sean Red•635
    @seanred360

    The answer your question: React does not have an opinion about your css. It is a javascript library that keeps the virtual DOM in sync with the real DOM. You can use css, scss, styled components, inline styles, styled JSX, React does not have an opinion about that.

    IDs are used in CSS to make a rule more specific than a class. I rarely use IDs because I rarely need to have something so specific. I always use class as a default. If I for some reason need to target something very special that is one of a kind in the whole app, I might use an ID. An example could be maybe you have many buttons on your site with the classname of 'btn'. You want them all to be the same size and font, but you have a very special submit button that you want to change it to be green. You do not need ID for this. You can put the class 'btn' and another class called 'green' on it. Then in CSS you can target '.btn .green' to target that button. Then if you went a step further and you have several buttons with '.btn .green' you could target one of them with an even more specific rule like an ID. The ID will override the styles in '.btn .green'.

    In conclusion, using IDs is a personal preference. I do not like to use them because if I begin with an ID, how can I get a more specific rule above that if I want to override the styles?

    Marked as helpful
  • Sean Red•635
    @seanred360

    The site itself looks really good. I am really curious why you di not use JSX. I have never seen someone do something like this. I find JSX is a lot easier for other developers to read. Also can you explain why you want to put a button inside a button? A button is not supposed to be put inside a button because when you click one button the onClick event will bubble to the other element and it will also get clicked. So a button in a button behaves like 1 button always. Why not make 1 button that has a <span> inside and put whatever you need inside that?

  • Sean Red•635
    @seanred360

    You app looks great! I like the purple theme you made. Your site throws an network error because it can not find a manifest.json. I added a manifest and favicon support for most devices. In a web browser the favicon will show on the browser tab. On Iphone and android it will show as an app icon. You should always add a favicon and a manifest, it helps different devices learn about your app. You can check my pull request on Github for details.

    Marked as helpful
  • Sean Red•635
    @seanred360

    Very nice implementation it looks flawless on the outside. Under the hood it looks like your HTML is a mess. If you look below the screenshot section of this page, will will notice you have 253 accessibility issues and 254 html validation issues. You should not ignore this. You have very major problems like <div> tags that are not closed or giving many elements the same ID. Some browsers will not be able to figure out how to display your pages. Thankfully the view report section explains each problem and how to fix it, so it is not a big deal.

    Marked as helpful
  • Sean Red•635
    @seanred360

    You have an infinite loop in your Advice.js. Currently you did not add a dependency array to your useEffect hook. This will cause the Advice component to repeatedly fetch the data many times per second.

    what you have: useEffect(() => { (async () => await fetchItems())() })

    when you add a dependency array: useEffect(() => { (async () => await fetchItems())() },[]) <------the [] here

    a quick fix for this is to simply add your fetchItems function into useEffect and add an empty dependency array like this so that It runs once. Like this

    useEffect(() => { const fetchItems = async () => { try { const response = await fetch(Api_url); const randomSlip = await response.json(); updateAdvice(randomSlip.slip.advice); updateID(randomSlip.slip.id); } catch (err) { console.log(err.stack); } }; (async () => await fetchItems())(); }, []);

    When you add a dependency array you are telling useEffect to only run when the variable you put in there changes. Otherwise it will just run a million times a second repeatedly. If you have an empty array, you are telling useEffect to only run one time when the component renders. Unfortunately in React 18 this no longer works this way. An empty dependency array will run twice, which means you are fetching and rerendering twice. React does not want people to fetch data in their useEffect hooks anymore. We should move to using Suspense. I will make a pull request on your Github and show you what this looks like.

    Marked as helpful
  • Sean Red•635
    @seanred360

    I recommend you use object destructuring in ALL of your components, you have a lot of duplicate variable names and collisions. I was very hard for me to understand what was going on. Pretty easy fix though. Check my pull request on Github for details on how to do that. Good job so far, you seem to grasp Javascript pretty well. You just need to learn the best practices for React.

    Marked as helpful
  • Sean Red•635
    @seanred360

    You no not need to fix your handling of deleteReply. For this small project what you did is fine. If the goal was to create a massive scalable database like Facebook has you would normalize the data, which means you would make all your data 1 level deep. So you would have a collection called "comments", that is an array of comment objects with the id, content, author, and the unique IDs of all the replies, but not their content. You would have another collection called 'replies' that is an array of reply objects with author, content, and the unique ID of the comment that you are replying to. You would have a collection called "users" that only an array of user objects with a username, photo, date joined etc. The reason you have everything spread out is because it would not be possible for 1 user to download every post and user's data on the entire database of Facebook on load. You have to put everything in small separated collections so you can only grab what you need. So when you want to fetch your replies you look at the replies IDs array in your comment and fetch only the replies from the replies collection that have the ID contained in your comment.

    Marked as helpful
  • Sean Red•635
    @seanred360

    I would also use a different design for your page routing. You should have a login page and a homepage only. You need to make a "protected route". Basically when the user tries to go to the homepage, the router checks if they are logged in, if not it will not allow them to go to the homepage. Instead it will redirect them to the login page every time they try to go to the homepage. You can see this behavior in my project here. https://www.frontendmentor.io/solutions/full-stack-mern-react-firebase-rest-api-mongodb-L08z7tmdo

    This design structure also allows you to forbid a user from viewing an account page that they do not own or an edit page for a comment they do not own.

    edit: I see you do have a protected route, I would make the hompage the default "/" and login page as "/login"

    You should change your routes to something like this:

    <Routes> <Route exact path="/" element={ <Protected> <HomePageNameHere /> </Protected> } /> <Route path="/signup" element={<Signup />} /> </Routes>

    and change Protected.jsx to

    const Protected = ({ children }) => { const { user } = useAuth(); if (!user) { return <Navigate to="/signup" />; }

    return children; };

    Marked as helpful