@K0smicSubmitted almost 3 years ago
I finished the calculator, trying to stay as faithful to the design as possible.
I'm always looking for tips for code, readability, and optimization. A look at the code is very welcome.
Aymin
@ayminahmedpkAll comments
- @ayminahmedpkPosted almost 3 years ago
100% pixel perfect on the desktop and nice working calculator. Nice ux detail with the default text being darker as well. Having said that, it's not perfect for mobile. It's obvious looking at the desktop that you could do it for mobile too, but since it's for practice, might as well implement it.
You asked about code readability and optimization. Well, I looked at your sass and there's a couple of things you can do to simplify it. Firstly, even though it is clearly commented so I know what section is responsible for what, it is still kinda long and scary. If you split it into smaller partials and imported them into one main file, that would we much cleaner. Also, try SASS lists. With lists you can define all the color variables in one place, and then write color styles only once, and use @for to generate 3 different styles for 3 different themes. Makes the SASS much shorter.
Your calculator code is very short (at least compared to mine) and works well. One thing you could do is, instead of sending out the normal output, you can have the output go through .toFixed(3) if it is a fraction, so if someone calculates 1/3 it won't be 0.333333... outside the calculator display.
You are using 'eval' in your JS. I've been told by the internet to simply avoid it since it is too dangerous, so you might want to avoid it in real life. I made a calculator once using eval too, so I made it again, this time without eval just to compare my new code style.
Also, just like with SASS, your JS has the theme switching code and also the calculator code in the same place. I would suggest you try to separate these files since they relate to different areas of your app. Smaller files will allow you to reuse your code easily, such as the theme switching mechanism for another site, etc.
Oh yeah, one more thing. You could try using cookies or local storage to save the current theme in browser storage to persist between page refresh. It is just an optional objective for this challenge, but it's kinda fun if you try it.
Finally, if you want to see an example of all of the above, check out the repository of my solution. I have continued to work on it even after I submitted it (cleaned my SASS with lists just now), and may resubmit it too. It's not as pixel-perfect as yours, but you can see the things I was talking about regarding code.
All the best.
Marked as helpful1 - @cxkeeleySubmitted almost 3 years ago
Any feedback is very welcome, I've learned some solutions from PhoenixDev22, such a great mentor to follow, waiting for your comment bro, cheers, and happy coding
@ayminahmedpkPosted almost 3 years agoLooks decent. However, there's a few ways you can raise your game.
I noticed there is an animation when moving from mobile to 'laptop layout', but not when moving to the desktop layout. Consistency is actually a big part of UX. Try to have the animation in both cases or in neither case, because it's jarring when it only happens on some occasions. But if you're just experimenting, then congrats, it works and looks ok.
There's a few other things you could do too. You can make the design more accurate by using PerfectPixel extension on Chrome to fine tune your style, and you can use Figma to map out the image so you know the exact fonts and box sizes.
Also, consider using Grid, that way you can get rid of the 3 'left', 'middle' and 'right' divs. It'll help you simplify your markup as well as your CSS.
Finally, if you're going to restart and resubmit this challenge, look into BEM as well.
If you look at my profile, I have submitted this challenge with a Figma file (my own, not from FEM), so you can get it from my repository and use that as an example. Also, I've made a previous comment for this same project explaining all of this in more detail, so that might help you out too.
BTW, I did this challenge twice. My first submission was with Flex, and the second one was with Grid, so if you want to compare the code you can check my commit history for that repo.
Marked as helpful0 - @elvinasnSubmitted almost 3 years ago
Hello, I did this challenge using flexbox, is it okay, or it is better to use grid? Thank you. Also I am open for any other feedback and suggestions.
@ayminahmedpkPosted almost 3 years agoHi Elvinas.
Your projects looks similar to the design which is good. You also use BEM already which is pretty cool. That said, there are ways you could raise your game.
Firstly, I noticed your media query targets mobile with the max-width after you've already set the desktop styles. I would recommend you to always design mobile first, as in write the CSS for the mobile version, then add the media query to target the next version, usually with the min-width. This is the convention which should also provide better performance for mobile devices. Personally, I find it is much easier to expand a layout than to contract it. Not to mention it is annoying to have to work on the mobile version properly when you know you've already met the screenshot requirement.
Secondly, while your layout is fairly similar to the image, it is not pixel perfect. The cards are larger and not aligned, the fonts aren't aligning properly with the image, etc. That may or may not be a requirement in real life, but since these projects are for learning, I suggest you try to make it pixel perfect or as close as possible. There's a very very useful Chrome extension named "PerfectPixel" by WellDoneCode which I use to help with this.
However, the easiest way to make your design more accurate is to first map out the image in a Figma file (or XD or whatever software you would use). That way, you already know all the font settings for each section perfectly and also the exact distances between every div on the page. It's an absolute pain to use trial and error to figure out the fonts or any CSS property like paddings, margins, etc. when developing.
Furthermore, you ask about whether grid would be better. Grid will most likely make your markup flatter and clearer - you probably won't need that
<div class="sections__column column__tworows"> and so on. Grid takes most of the layout work out of the markup and into the CSS. Also, imagine you looking at your code 4 months later and trying to remember what's going on. Unlike flex, the CSS for grid will be a lot easier to understand at that time, especially if you use grid-template-areas and visually represent the layout in your CSS. I know it's a burden to have to learn another layout tool when you already have flex, but trust me that it's very useful for 2-dimensional layouts and you'll be learning it eventually anyways. So while you're here learning, use this as an excuse to get friendly with Grid too.I would recommend you do all of the above and any other improvements you can think of yourself, and then start this challenge and submit the updated solution again.
I actually submitted this challenge with Flex yesterday, then added Grid and submitted again. (you can see changes to the markup and CSS that I was talking about in my commit history). Also, the my figma file is also in my repository. This is not FEM's figma file, this is my own, but when you look at it you'll see how clear it makes when you can map the layout and know the exact font settings and div dimensions. Oh, there's also some notes in there in learned.txt where I talk about the laptop layout, etc. You may find some of these useful.
Lastly, if you think re-doing the same project will be boring and slow you down, think of it this way. You are here to improve front-end. What difference does it make whether you do it in 4 projects or one? If you can make improvements to your code, learn and compare new techniques in one project, then you are just getting more value and benefit out of a single project without having to start another one from scratch.
All the best.
Marked as helpful0
