Not Found
Not Found
Not Found
Not Found
Request path contains unescaped characters
Not Found
Request path contains unescaped characters
Not Found
Not Found
Not Found
Request path contains unescaped characters
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

All comments

  • DLyons 20

    @drding00

    Submitted

    I tried for pixel perfect on this one. Hopefully I've gotten pretty close. My working screen resolution is 1920px so I ended up adding a dark background to the site because the specs stated a max width of 1440px. I think it works pretty well with the overall color scheme.

    This should be fully responsive, so absolutely test it on different devices and operating systems if you can (I myself only have access to windows and android).

    Any and all feed back is welcome! Thanks for your time!

    @steventoben

    Posted

    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!

    2
  • @steventoben

    Posted

    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!

    1
  • Luis Colina 1,043

    @Comet466

    Submitted

    hi, hope that you day is going great, this is my first css challenge, i'm relative new in the web development (started learning on january of this year) soo any feedback is welcome with open arms

    @steventoben

    Posted

    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!

    3
  • Aymin 140

    @ayminahmed

    Submitted

    Development certainly becomes much easier if the image is first translated into a figma file to clarify the margins / paddings. More time (and energy) is saved during the development process than in expended in translating the design.

    Don't like using fractional rems, seems hacky. Am I visually splitting the spacing incorrectly, and ending with wrong margins / paddings for various components?

    Will be grateful for ANY feedback.

    @steventoben

    Posted

    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.

    2
  • @steventoben

    Posted

    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!

    1
  • @steventoben

    Posted

    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!

    0
  • @steventoben

    Posted

    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.

    1
  • Jen 1,230

    @En-Jen

    Submitted

    Hey everyone, I'd really appreciate some feedback on this one. This was a really big project for me since I'm still pretty new to React and it's my biggest React project I've developed. It was also my first time styling a React project with styled-components. I really enjoyed the sass-like experience of using styled-components and being able to conditionally style elements with JS (very useful for the different colors with the two themes).

    The main thing that still needs fixing is that when the user drops a todo item after dragging it (which is implemented with the react-beautiful-dnd library), sometimes there is a brief flicker on the items. Has anyone encountered this before in your projects and have you been able to fix it?

    Another thing I'd love feedback on is the structure of my project. Should I be splitting my elements up into even more components? Some of the components I created are strictly css (with styled-components) and don't involve any React. I did this for things that are repeated a few times throughout the project, like for buttons with images in them and for elements wrapped in a box with the same border-radius, box-shadow, width, padding, etc. Is that an appropriate use of a component?

    Any other feedback you have is totally welcome!

    @steventoben

    Posted

    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

    0
  • @ALapina

    Submitted

    Project was built with maximum accessibility in mind. Go and try clicking and tabbing through buttons! You even can try voice over!

    Also, check out how I use pseudo-class :focus-visible to remove focus outline on mouse click on button, but add and make it visible on tabbing through buttons.

    I build two variations - first with simple Vanilla JS and second with React. For a small component like this React is probably overkill, but as a complete beginner for me it was challenging and I learn a lot about React with hooks!

    Repository URL for React solution: https://github.com/ALapina/FAQ-Accordion-Card-React

    Live Site URL for React solution: https://alapina.github.io/FAQ-Accordion-Card-React

    @steventoben

    Posted

    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!

    0
  • @steventoben

    Posted

    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.

    0
  • @steventoben

    Posted

    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.

    0
  • KT 390

    @KtGitIt

    Submitted

    Greetings all,

    Any suggestions and feedback will highly appreciated on anything.

    It looks like I am having hard time moving on from "px" to relative units. As I was using % for some, everything was moving like crazy when I was resizing the browser. Any guidelines on how do I incorporate more relative units in projects?

    Best regards, KT

    @steventoben

    Posted

    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.

    0
  • @steventoben

    Posted

    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.

    1
  • @Nedjeljko-Miloslavic

    Submitted

    This is not fully finished project. I might return to it later. Right now, if you change tabs my animations won't be in sync. This is probably a way browsers spare CPU from over-work. There is a way to fix this with JS, but I would have to change a lot of code and, for now, i'm gonna leave that for later. But if some of you have a desire to experiment with my code go ahead!

    @steventoben

    Posted

    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.

    0
  • @Aayman-star

    Submitted

    Hi there, this is my first attempt with html and css,I haven't applied the media queries yet and the color scheme is a little different as well. but still hoping for some reviews:)

    @steventoben

    Posted

    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.

    0
  • @AbdullahiAA

    Submitted

    It was a nice experience for me working on this project.

    The question I have is that: how can I position the two circles on the background so that it stays at a particular side relative to the view-port?

    @steventoben

    Posted

    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.

    1
  • @astroud

    Submitted

    👋 I will be updating this app (refactoring state using Redux, writing more tests), so I am interested in any and all feedback!

    • The timer mode is controlled by a form containing three radio inputs. There's no submit button because the form is never meant to be submitted. What's the best approach to this accessibility issue?
    • Why is Firefox rendering my colors differently than Chrome/Safari?
    • Is there too much transition bling on the buttons?

    @steventoben

    Posted

    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!

    3
  • P
    ApplePieGiraffe 30,545

    @ApplePieGiraffe

    Submitted

    Hello, everyone! 👋

    This is my first challenge with React and as usual, I learned a lot! 😆

    At first, I was pretty confused about how to organize the files in my project and I'm still unsure if my React is very clean in some places (since I just began learning React). My Sass turned out to be, well, kind of messy (and I'll keep in mind to use a better file structure next time because I simply created separate files for my components for this project). 😅

    On the bright side, I created this pretty sweet hover effect for the cards in the "Creations" section from scratch, and I quite like it. 😊

    Any feedback (especially on React) is welcome and appreciated! 😀

    Happy coding! 😄

    @steventoben

    Posted

    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.

    3
  • P

    @emestabillo

    Submitted

    Hi everyone, had a challenging time with this one from start to finish. It's my first multi-page and react project and I thought the design was a fun way to practice mapping. It quickly became one issue after another (Where to put CSS? Module or 7-1? How to access the images? Will they be in public or src? How to NOT access the DOM?) On hindsight, I should've studied a bit more before this attempt. Still very much a beginner.

    The CSS was equally hard - there are layout shifts in every section of every given screen design. The form jumps with validation and I'm not able to implement the dropdown design for the select choices, but I think I'm ok with these. Needless to say, I learned A LOT. Big thanks to @grace-snow for accessibility tips :-)

    For my questions:

    1. Is there anyway to reuse a Button component in such a way that you can control its parent tag (a or button) or pass it as props? I'm reusing the button classes for the form button which is not very DRY

    2. How do I reset the form on successful submit? e.target.reset() isn't working.

    3. I tried to create a custom hook for responsive bg headers. Can't figure out why the images are not showing. Here's the relevant file if anyone would like to take a look.

    4. The events section on the homepage has this light gray bg-image that would only render as a square block if I used the appropriate image (curve-top-right.svg). Luckily, the other assets look similar so I just rotated it (curve-top-left.svg) to get the same effect. Can't figure this out.

    Any other feedback is welcome. Thanks in advance!

    @steventoben

    Posted

    Hey this looks great I'll answer your questions first. For the first questions, yes you can do that. I personally do that for my Typography.jsx components. It basically looks like this:

    const Typography = ({as, children, ...props}) => { return React.createElement(${as}, props, children); }

    So you could apply the same logic to a button. Basically what I'm doing is creating a functional component with the props de-structured, so you'll have access to the 'as' prop, which is just a string of what element you want to create. You also have access to the children and all other props passed. So <Typography as={'h2'} className={"Display4"}> Some text </Typography> would come out rendered to the dom as <h2 class="Display4">Some Text</h2>. So that's an approach I like using, you could just use an if statement and check if the prop exists. I have a React UI component library and my Buttons are extremely customizable and reusable, reusable code and styles is such an important part of front-end development.

    For resetting your form you could create a ref for your form DOM node and then you would be able to access the form on the actual DOM and call methods. So you could do ref.current.reset(); If you're using React state for the values you want to reset, you could simply just set the state to it's initial value, you could keep an initial value in a ref (instance variable kind, not DOM node kind) I couldn't quite tell how you were controlling the form's state.

    It looks like you're attempting to update the value of the image in a function in your functional component. You'll need to put that function inside of a React.useEffect hook, and i'm guessing the dependency array would be window.innerWidth, unless you're rendering from the server. Try putting it in a useEffect hook like I said so basically every time the window width changes, it will call that function and check with the new value. You could also wrap it in a Memoized function since you only want to calculate if the one variable changes, but I'd go for the useEffect approach first.

    Ok sorry I'm not exactly sure what you mean by this one. Not sure how much help I can be with this one but off the top of my head if it was rendering a square but you wanted a curved part then I'd try to set the clip-path property on it's class or the mask-image. Clip-path is really useful and I use it quite a bit, it let's you make shapes out of your box model, you can do circles, ellipses, polygons, and I think arcs. There's a lot you can do with that styling rule so maybe it could work, I'm not too sure sorry tho.

    So yeah now onto just some general stuff, first off don't set font-size to 62.5% in your html tag. I have to tell people this every single project lmao but you should either set font-size to 100% or don't add the rule and let it be inherited from the browser settings (either way it should default to 16px). I know people use it so that 1em=10px but really, 16px is better anyways plus you don't mess with the user's accessibility settings. Also i find it kinda odd that you're using sass, you even made a partial for variables, but you're using standard css variables lol. Also just a quick typography lesson because every single person uses the properties incorrectly; font-size (unless you're initializing root size to 100%) should always set in rem units and idealy you should have a typographic scale set up but that's kinda overkill for these small challenges. Letter-spacing should be set with em units. Line-height should be unitless. It's a multiplier of the font-size, typically paragraphs have a line-height around 1.5, and headings are generally lower at around 1-1.25. Also I would try to avoid setting font-styles to tags like h1 or p, because usually you'll have many different sets of headings or body classes, and you don't want to be that broad with selectors. Also good jobs measuring all of your widths and heights and paddings and stuff in rem units, that's good practice. I don't know where a lot of the sizes are coming from tho, there seems to be a lot of magic numbers in there. This is just a personal design rule but I try to make sure I never see a number in my sass. If it's an important number it will be a variable, if it's not in my set of variables (which are limited and meaningful, which lets my pages have vertical rhythm and a sense of harmony) then there's probably a better way of sizing or positioning something. But that's just me being pretentious because I like UX lol. The last thing is I recommend to try to reduce the amount of nesting your do with sass. Nesting too much can make your compiled css file size huge.

    Oh and one last thing, there definitely are annoying html tags that won't let you restyle them, especially the select and options. That doesn't mean you can't make your own custom dropdown tho. I have like 10 different variants of dropdowns, you just have to make sure to correctly set the attributes saying that it's a select or an option. I would definitely have done that in your case because mapping out all of those options would have been so simple compared to listing them all out. I often make custom textfields too because they always look much better if I have a fake placeholder element and a label element that animate, it increases people's interest quite a but. Sorry I rambled so much lol this was just a pretty great looking project and it uses React which I love lol. Great job tho I think overall this looks great. Good job!! Let me know if you have any questions!

    3
  • @bwhitney2439

    Submitted

    All Feedback welcome. This one took me a while for some reason.

    Shout out too https://github.com/zuolizhu/github-jobs-frontendmentor for helping me with infinite scroll in react query and being able to hide load more button if there were no more items to render next.

    Shout out to Kent C Dodds article on using CSS variables for theme and dark light mode as per this article https://epicreact.dev/css-variables which appear to be more performant than using context provider.

    @steventoben

    Posted

    So your trying to consume the API with the wrong endpoint. You're using cors-anywhere.herokuapp.com as the host. That's not where the file is hosted, plus that app was for development purposes only and it looks like it's been shut down due to how many people were abusing it. So it looks like you're trying to consume a REST Api, by calling the GET request. When you send this request your header should have the scheme of https, your host should be jobs.github.com, and the filename should be /positions.json. + your query string you were using, except if there's no selection you should probably put an * to not filter everything out when it loads

    0
  • @samuelpalaciosdev

    Submitted

    Hey✨ hope everyone is okay. Just finished this using Flexbox and Grid , any feedback will be appreciated.

    @steventoben

    Posted

    I think it looks pretty good, but I think you used way to much grid and flexbox, It seems pretty unnecessary in a lot of places, especially with a pretty basic layout like this. Also I'm not sure what's going on with your font sizes. You shouldn't set the base font to 62.5%, and your font-sizes for the same exact elements are different throughout. Some buttons have larger font-sizes than others, there doesn't seem to be any form of a consistent typography scale being applied throughout

    0
  • Mehdi 70

    @mehdilehmamy

    Submitted

    how much can my css code be better, I know it's inefficient but I don't know to what degree. 500 lines of css for a simple landing page is a bit too much I think.

    @steventoben

    Posted

    There's a lot of incorrect html semantics going on. Your nav should hold anchor elements since a nav is supposed to be your main source of navigating throughout your page. On your hero section I'd change your flex's justify content to space between or something similar. You could also make the paragraph under the main h1 more legible by setting a max-width to something like 60ch. That would probably help it from overlapping with the image. Also you should only use one h1 element on your entire page. You're using far too many. h1 is the most important heading and then you should work down to h6, which is the least important heading. I also think you're abusing flexbox quite a bit. You could definitely use different display types throughout and it would work just the same. Anything that looks like a button, should be a button or anchor element. Also on the very bottom in your footer, you should use an unordered list for all of those lists. And all the list items inside the list should be anchor element because they represent links. Overall tho I mean it looks nice, it think the stylesheet is long because of the way you implemented your media queries.

    0
  • @steventoben

    Posted

    There's lots of problems with your html markup. You're setting font size in pixels which is bad practice. Your main has a class name "section". An img element always needs an alt for accessibility reasons. Your section element is not a section element, you should make it a div. Also the use of grid on your main component is completely unnecessary. You should try to stay away from absolute positioning, especially with weird magic numbers that make no sense. It's good that you're using rem, but because you set the root font-size to 12 your rem values end up becoming really weird numbers with like 4 decimal places. You have a div with a classname "header" which holds an h1 element. You don't need to wrap that h1 component. The use of flex for the whole accordion area is unnecessary and leads to problems with the entire component changing size on click. The use of article is incorrect semantically, for accordions look into summary and details html elements. There's also really no need to be wrapping your p elements with divs that are displaying as flex.

    1
  • @steventoben

    Posted

    How are you trying to center the card? Along the y axis? or x axis? or both? There's tons of ways if you just want to center the card to the middle of the screen just create a div that wraps everything in the body. Make sure the width is 100vw and the height it 100vh. Then set the display to flex. set justify-content to center to center is horizontally, set align-items to center to center it vertically. Flexbox is probably overkill for this tho it could most likely be done simpler if you clarified what exactly you want to achieve.

    0