Skip to content
OverviewSolutions
0
Comments
34
Steven Toben
@steventoben

All comments

  • DLyons•20
    @drding00Submitted almost 4 years ago

    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!

    Loopstudios Project

    1
    Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
    @Comet466Submitted almost 4 years ago

    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

    Css grid price tag, newbie edition

    1
    Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
    @ayminahmedSubmitted almost 4 years ago

    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.

    flex-direction, flex-align, justify content

    2
    Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Steven Toben•750
    @steventobenPosted almost 4 years ago

    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-JenSubmitted almost 4 years ago

    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!

    Todo app | React | styled-components | Drag and Drop

    3
    Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Anna Lapina•125
    @ALapinaSubmitted almost 4 years ago

    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

    FAQ-Accordion-Card JavaScript and solution with React

    2
    Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
  • Steven Toben•750
    @steventobenPosted almost 4 years ago

    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
    @KtGitItSubmitted almost 4 years ago

    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

    HTML and CSS With flexbox for mobile and Grid for desktop is used.

    1
    Steven Toben•750
    @steventobenPosted almost 4 years ago

    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