Steven Toben
@steventobenAll comments
- @drding00@steventoben
This looks pretty nice, I'm getting black bars on the margins which you mentioned. I would say don't go for pixel perfect. That's not a realistic thing to do, you shouldn't worry too much about sizing and everything. With that said, go ahead and remove the max-width: 1440px property you have set. When you do it looks perfectly fine. There's no need to limit the max width just because the design guide said to. It looks much better imo without the black bars. While on the topic of black, personally I don't like such deep blacks in my design, my text is usually #2f2f4f all the way up to #708090, but that's not really relevant.
The only other thing I'd say is make your animations faster. Feedback animations, such as hovering over a link should be in the ~200-300ms range (just my opinion). You'd be surprised how much of a difference it makes. It makes the webpage feel much cleaner and more responsive.
Buy overall I think this looks great! Good job with the responsiveness, it doesn't seem to have any breaking points so nice work!
- @mailsonsoares@steventoben
This looks nice I'd just address a few things. First thing is I would set font-size in rem units instead of pixels, this helps both accessibility and responsiveness. Second thing is on your paragraph text, instead of using <br> tags for every line-break you could set the max-width of the paragraph to something like 50ch. This is a really common strategy to make paragraphs more readable and easier on the eyes. Another thing is giving paragraphs line-height of around 1.5 (unitless). One more thing I'd do it clear the form when the submit button is pressed. Currently when you press the button the fields don't have their values cleared.
Other than that I'd say it looks good, for places to improve I suggest using rem for font-sizes and other measurements. Another thing is try to size things implicitly by setting a max/min height/width and letting the inner content size the box, instead of explicitly setting widths and heights. Good job overall it looks really nice!
- @Comet466@steventoben
Good job on your first CSS challenge! A couple things I want to point out just in case you didn't realize because they're useful tips: On your 3 divs that are children of the grid container div, you don't have to explicitly set a height. Removing the height will make it take up the size of the row of your grid, which is pretty much the same size as you set. Nice job of using grid-areas and naming them. For your 2nd and 3rd div you actually don't even need to set the grid-area property because with the way grid works, the next child will take up the next grid area available. So if you removed the grid-area: box-2(and 3) it would actually result in the same thing. Just a couple useful grid tips I thought I'd share.
When you do set something's height, try to use max-height or min-height (same goes for width) and try to set it using rem units. Rem units make the application much more responsive and it helps with accessibility. Good job using rem everywhere else tho like on padding and border-radius!
Nicely done, especially for your first CSS challenge! Just thought I'd share some tips about css-grid and rem units!
- @ayminahmed@steventoben
Fractional rems may seem hacky but it's actually the best way to size things, and the values don't have to be so exact. Just for fun I tried adjusting the styles on a card with clean styles and it looks pretty much the same (at least on my screen). On the div with className "category-card card-1", I applied flex in column direction to that card and the styles became a lot cleaner. I got rid of the width, height, margins, padding values and set padding to 2rem and margin to 1rem. Then on the img inside that card I got rid of the absolute positioning and set order: 1, align-self: end, margin-top: 4rem and that looks essentially the same.
This looks good and is pretty much spot on to the design, and there's tons of ways to go about styling things (and the way I did it may not actually work, I was just trying to show that there's tons of ways to go about styling). Being exact isn't necessary, it's ok to round your rems to the nearest whole number or 0.5. I'd definitely recommend trying to not explicitly set width and height, especially in pixels. If you can, setting a max (or min) height/width in rem is a good way to go.
I think this looks nice, I don't think being so exact is necessary. You could try rounding rems on your next project, if you don't like it then you can use the exact values if you feel more comfortable with that.
- @naglaa1@steventoben
I would recommend not setting each section's height to be the size of the viewport. It's best to let it be based on the size of it's content, such as the size of an image. In the white box on the second section containing the heading and paragraph I would suggest adding some padding to the bottom of the box, at certain sizes there is a gap where the picture is showing through because the box is too short. I'm not sure if you noticed this but when you hover on an image in a row on the last section, all of the text in that row changes color, not just the hovered one.
I would suggest putting the heading for each section as a direct child of your section elements, as this is how section should be used semantically.
The last thing is I would suggest taking a look at your stylesheet. Try to avoid nesting more than 3 levels deep, this can cause extremely long stylesheets to be generated when your sass is converted to css. I'd also suggest purging your icons stylesheet, there's thousands of icons but you only need a few so that would free up a lot of overhead.
Overall I'd say it looks nice, good job on this big challenge!
- @naglaa1@steventoben
I would recommend not setting each section's height to be the size of the viewport. It's best to let it be based on the size of it's content, such as the size of an image. In the white box on the second section containing the heading and paragraph I would suggest adding some padding to the bottom of the box, at certain sizes there is a gap where the picture is showing through because the box is too short. I'm not sure if you noticed this but when you hover on an image in a row on the last section, all of the text in that row changes color, not just the hovered one.
I would suggest putting the heading for each section as a direct child of your section elements, as this is how section should be used semantically.
The last thing is I would suggest taking a look at your stylesheet. Try to avoid nesting more than 3 levels deep, this can cause extremely long stylesheets to be generated when your sass is converted to css. I'd also suggest purging your icons stylesheet, there's thousands of icons but you only need a few so that would free up a lot of overhead.
Overall I'd say it looks nice, good job on this big challenge!
- @vishalnirmal@steventoben
I haven't looked at the code but the implementation is nice. Just one thing I noticed that should be changes is with modals and dialogs. A modal should take over your screen and hijack the scrolling. Essentially when a modal or dialog is opened you shouldn't be able to scroll the main page that's behind the overlay. Modals and dialogs require an action to be made and you shouldn't be able to interact with any other element on the page including scrolling the entire page or pressing buttons and stuff like that. The popup is the thing that requires immediate attention so it needs to be interacted with and stay in a fixed place until you interact with it.
- @En-Jen@steventoben
This honestly barely looks like a React app to me. The styled components is completely unnecessary and makes the code almost unreadable. Some of your components aren't even React components, it's literally CSS applied to an HTML element in a JS file. That's a massive anti-pattern. You want to separate every part of an app as much as possible. Your React components don't really make sense to me, the point of React is atomic design, allowing for reusable components. You could have made a button component that would be used for all other instances of a button you had throughout the code. You should separate components by domain, down to the most basic level and build up. Regarding styling, I already say this to every single person on here so I'll keep it short; don't set your font-size in the html tag unless you set it to 100%(which is the same as doing nothing). And then of course I have to ask, where did your numbers come from? You heights, widths (which usually shouldn't be set at all), padding, margin, etc. The numbers look completely random, CSS requires logic to properly layout a design and make it maintainable. If you change a couple numbers the entire design is ruined. Also a lot of those effect hooks look like they aren't really cleaning up properly. Last thing is don't use flexbox for everything. There's a lot of hacky html with incorrect semantics, as well as hacky css, I'd try to make sure everything is properly setup you're not relying on unstable solutions
- @ALapina@steventoben
The dl, dt, and dd tags seem unnecessary to me, use details and summary for accordions instead. If you're going for maximum accessibility, don't set font-size with pixels, or really just don't use pixels at all. Use relative units for almost everything. Also the entire containing box grows when an accordion is toggled open. And one small detail is that an "Accordion" component should only allow one (or zero) in the set to be open at once. But it looks good otherwise!
- @AlekseyVY@steventoben
This seems incredibly over-engineered, using React is fine if you want but this app has barely, if any state that's not local. There's barely any state in general. That seems like a huge waste of bundle size by adding all those extra libraries that really add nothing except complexity.
- @anastasssia88@steventoben
I'm really confused by your css code. Are you using both tailwind and bootstrap? You have over 200,000 lines of css in your app. You should really look into that because that's an insane overhead especially for a project that needs maybe 100 lines of css. Also why did you use UUIDs? I don't see any logic in using them here, just starting at an id of 0 and iterating up would be much faster and make more sense. You can access any object in the array if it's id matches the index and that makes sense to do in this app.
- @KtGitIt@steventoben
The main relative unit you should be using is rem. Rem units are relative to the size of the root font-size, which is usually 16px by default but can be changed in the browser's settings. User rem to set font-size, padding, margin, dimensions, etc. It's the main unit to use because it will scale to whatever the user has set their font-size to, so it helps with accessibility. Em is similar but it's relative to the selector's font-size instead of the root one. The value of a rem won't change, but the value of an em can. Em is really only useful for a couple things like media queries, letter-spacing, and if you want something relative to a component's font-size. vw is the width of your window, vh is the height of your window, so those could come in handy on occasion. Ex is the size of the lowercase 'x' of your font and it's not used for much. Ch is the width of the '0' and can be used to limit the width of paragraphs based on rough character limits. Rem is going to be the main unit, and most simplest unit to use. I would definitely stay away from any absolute units in almost every scenario.
- @kirushenski@steventoben
One thing I'd suggest is memoizing components, especially todolist items. Every interaction with anything on the todo list rerenders every single component in your tree, which is pretty unnecessary when not much will be changing.
- @Nedjeljko-Miloslavic@steventoben
In regards to your animation being out of sync when switching tabs, that has to do with the browser's execution stack and the use of RequestAnimationFrame. RAF fires at 60fps but to optimize performance, if the tab is not being viewed the tick rate is throttled to around 2fps. If you want you could just keep the state of the timer and use the visibility API to check for when the tab regains focus and catch the value of the display up to the state.
- @Aayman-star@steventoben
I highly recommend not changing colors unless you know what you're doing, the contrast ratio makes the text illegible and basically every piece of text is firing off accessibility warnings. There's quite a lot going on here but basically you should use better html semantics and also use class names much better. You have a header (that's not actually a header) named top, which really has no meaning at all. Also I wouldn't style entire selectors like you're styling h3 and h1 and stuff without using class names so you're changing it for everything in the document. You're also using h3 a ton I'm not sure why, you should use h1-h6 starting with h1 as highest importance and h6 as lowest. Since you're using grid on this theres really no reason you should be setting widths or heights to the grid items. Last thing is you should avoid using pixels, use rem units or another applicable relative unit when setting length, and also you shouldn't really be using magic numbers. Setting padding and margin on every element to random pixel values isn't necessary. Finally you should make your button an actual button, with a cursor of pointer, not a div. Interactive elements should appear interactive.
- @AbdullahiAA@steventoben
For your question: for the top left img style it with left: 0; top: 0; transform: translate(-50%,-50%); and do the similar thing for the bottom right one, right: 0; bottom: 0; transform: translate(50%, 50%); make sure they're both absolute positioned and that should lock their centers at the corners of the viewport. Make sure you add overflow: hidden; to the body or wrapper so you don't have the images hanging off the page causing it to scroll.
- @astroud@steventoben
Ok so in terms of React code quality, you should definitely work on code-splitting. In React a functional component really shouldn't be more than a few lines. So I'd recommend making more things into components, like you had a couple button html elements, those should be able to be a <Button/>. Speaking of that component, a Button component should return a button or anchor html element, on one of your cases your Button component returned a div with an input as a child, and your default return is null. I would at the very least just return a button instead of null, but even better you should use PropTypes to validate props and provide defaults. Another thing you could do to make your code cleaner is to make functions for each type of button in your Button.jsx, like a CloseButton, SettingsButton, etc. above your main Button function. Then you could check your prop and return <CloseButton/> or something like that all in one file. Another thing is making separate functions for each button, but before your export default Button line write Button.Close = CloseButton; Button.Settings = SettingButton; That would let you call a specific button from outside the file using dot notation, like you could call <Button.Settings {...props}/> in some other file to display that specific button. Last thing on buttons is you should style them with the line cursor: pointer; that way when you hover over them they appear much more clickable.
Like I said before try to code-split, you could turn your radio inputs into their own Component. The Header component doesn't seem right to me, a <header> is much different than a heading so <h1> doesn't match the name of the component at all. And for your mute toggle button, ideally you should be able to use a <Button/> to render that one.
For settings there's a lot going on here. It looks like you could make your own Text component, Input components, Label components, Form component. Pretty clever way of toggling the visibility of the settings modal, but it could be made even simpler by using a portal. The ReactDOM.createPortal() function lets you render whatever you put into the function, anywhere you want, so you can render things to document.body and then you'll have an element that is created as a sibling to your main VDOM tree.
So another thing is when you use one of React's synthetic events, like onClick and set it to an external function like you did with onClick={handleClick}, you need to handle state more carefully. Creating an external function is completely ok but when inside the function you shouldn't write setIsActive(!isActive); instead you should write something like setIsActive((prevState)=>{return !prevState}); This seems odd but React's state updates are async so sometimes it takes a while to change the state and you could be working with the wrong state. So using the previous state is a nice workaround. Also you might want to think about using a ref for some of your Component variables. If you have a variable that you declare in a component that you want to persist through rerenders, and is really only used for storing some values, like colors, React's useRef hook lets you create instance variables which is helpful because on a rerender, React will recreate everything. Every function, variable, etc, is recreated unless persisted in a ref.
So you mentioned you'll be updating the state with Redux, which is fine although I don't think it's really needed for this project. When you do implement redux, make sure not to do the same thing you did on this project. Do not make every single piece of state global. Most state in an app should be local, or hoisted up a layer, but most of the time global management for a state is unnecessary. I think you could use the Context hooks in React for any global state you do need, then send the state as far down the tree as possible. Ideally your state should be kept as close to the component as possible.
As for CSS, I'd also look into code-splitting. That's a pretty massive file and it's a very common approach to create a style sheet for each component, as well a global one. I'll just comment on a couple things here, avoid magic numbers, numbers that look completely random and have no meaning except for the fact that they "just work". Try not to be so general with selectors, like don't style an h1 element unless it's for a reset, use class names. I highly recommend you avoid pixels at all costs, trade them for rem units, it will make your app much more accessible and responsive. At the very least use rem units when setting a font-size, ideally they should be used for dimensions, margins, padding, etc. And I think that the animation spin on the buttons is a bit too much, try to keep animations subtle and quick, ideally in the 100-400ms range, probably never past 900ms.
Overall though the project looks nice, I would recommend taking these things into account for future projects though, maintainability could get out of hand pretty quick otherwise. Good job it looks pretty great overall!
- @ApplePieGiraffe@steventoben
For code quality I'd highly recommend code splitting. You're functional components are really large, generally a component should be broken down to the deepest level. It looks like you should break down Nav into it's own component, List, as well as list items. You should also use the mapping feature to map list items to a list so you don't have to repeat lines of code. It looks like you're using anchor elements and images enough to break them into their own component. On your hamburger menu the toggling state is overly complicated, as well as bad practice because state is async, so it could not be updated yet when that function is called. The best way to prevent that is just in your function writing setActive((prevState) => {return !prevState}); I'm pretty sure your useEffect hook is wrecking havoc on your performance, you have no dependency array so it's firing every single render, and useEffect is for managing side effects, so setting up the event listener to observe the window size is fine, but you must write a return statement that removes the event listener to clean up the effects. also add a dependency array to the hook containing window.innerWidth, that way it will only fire if that value changes. You could also consider debouncing since you really only want the value at the end of a resize. Also, you're not using props at all? props is one of the most important features in react and should be heavily incorporated in the app composition, instead of manually writing all of your data. Also one last thing, try to avoid styling like you did in the Creations functions. It looks nice but it hits performance heavy, especially an onMouseMove call, react will reconstruct all of the functions and variables at each render unless memoized or placed in a ref, so grabbing directly from the DOM over and over and repainting will cause some performance issues usually.
Onto the styles.... we've already talked too much about font-size lmao so let's just ignore that. In general your sass is pretty dirty. I'd recommend trying to compile your sass and take a look at the output of the css file, that would probably explain much better than I could. Your nesting is far too deep and often times unnecessary. Try to practice DRY and create a mixin for commonly used styles. If you have sass that's more than 3 levels deep there is something wrong with your method. Also it common practice to put media queries inside of selectors, not the other way around. As well as using em units for the media queries, also line-height should be unit-less, not measured in rem.
React and sass both have an atomic kind of methodology, so break down components as much as possible, use props and refs, it's ok to have hundreds of React components, and is actually quite common and really shows the beauty of React when you atomize everything.
Overall tho it looks good, you could try using css modules if you want to try a different method of styling, they're scoped to each component so you don't need to worry about clashing class names.