@lukakavtarraSubmitted almost 3 years ago
Check my solution. I'm still learning bases of front-end I would love to hear some advices :))
Joran Minjon
@DrKlonkAll comments
- @DrKlonkPosted almost 3 years ago
Hi Luka, nice to see you on the platform!
Some (unordered) things that stick out to me:
The centering doesn't really work on smaller screens, as Simone pointed out. Use margin auto, flexbox or grid for this to keep it nice and responsive.
It seems like the responsive.css is the same as style.css. No need to repeat everything in a media query, just the things that are changing.
The border radius of the card seems a bit off, I think it can be just one value.
Using rem instead of px is usually preferred, because it increases responsiveness and thereby accessibility.
Naming classes is hard, but can definitely be improved here. Try to come up with something that describes the content of the element. "Annual-plan" is a decent name in that sense, "div-button" is not. You might want to call that "button-group". Also "rand-text" suggests the text to be random, which it is not.
Naming things is super important, even more so when collaborating with others.
The annual-plan class can be centered more cleanly by removing
max-widthandleftproperties and introducingmargin: 0 2remor something similarMarked as helpful0 - @MSorJinxiSubmitted over 3 years ago
This is my first project and I would need a feedback because I don't even know what to ask. I'm learning how to code by myself from 2 months.
What mistakes did I made? What could I have done better? What should I be careful about in such design? How would you rate my code from 1 to 10? ;)
Thank you for your help and support :D
@DrKlonkPosted over 3 years agoHi Maja,
I agree with the others that CSS Flexbox is a great way to accomplish flexible layouts. I'd like to add Flexbox Froggy as an additional resource to practice with it!
Happy coding!
Cheers, Joran
0 - @AgataLiberskaSubmitted over 3 years ago@DrKlonkPosted over 3 years ago
Hi Agata,
Nice solution! Good to read (in your readme) that using a framework would do nicely here, especially with the reusable components. I fully agree! It is well worth picking up one of them.
I especially like the visually hidden headers on sections for our visually impaired visitors.
Well done and happy coding! Cheers, Joran
1 - @lanechangerSubmitted over 3 years ago
4/5/2021 update:
- 167160d 2021-04-06 | fix: removed the flex shrink on rating__stars to prevent the stars from wrapping (HEAD -> master, origin/master)
- 056d2a1 2021-04-06 | fix: changed justify-content from end to flex-end for better compatibility (chrome)
- 7ce075f 2021-04-06 | fix: restored margin-left to ratings layout
- b738a17 2021-04-05 | fix: was using backquote instead of blockquote
4/4/2021: This was a fun one where I made liberal use of flex containers as a way of dynamically spacing out the components.
Like the ratings section for example. Initially I was practicing a for loop on it and made it such that margin-left = 16px * (x - 1) where x is each rating component. So the first one would have margin-left of 0, the 2nd would have 16px, the 3rd would have 32px.
Which works better for future proofing if additional components were to be added. But I decided to make them flex and assume that only 3 items are used, so the top one got justify-content of start, the 2nd one center, and the 3rd one end with the extra space being shrunk dynamically as the viewport shrunk. And I really liked how that turned out along with how the other flex containers shrink. It was mostly done through flex: 0 1 <basis> which prevented them from growing, but allowed them to shrink once the viewport got smaller than their basis.
@DrKlonkPosted over 3 years agoHi Jeremy,
Nice job overall! Some things I've noticed:
The third rating box should be:
justify-content: flex-end(instead ofend).The stars wrap from 640-653 pixels wide for the middle rating, that looks a bit weird. Also, from 792 to 426 pixels wide the stars seem to move upon resizing. They don't align properly with the other ratings either.
All this is because the rating__stars resizes because of the flex-shrink, but the stars have a fixed width. I think it is better to remove the flex-shrink from this class.
Btw,
flex: 0 1 autois the default, so you don't even have to set the growth and shrink explicitly!Cheers, Joran
1 - @pikapikamartSubmitted over 3 years ago
This challenge was really fun. My first draft was full of animation but I couldn't pull that one formula in terms of checking boundary so I had to remove it. Limited myself to using only few html elements. So I need to be very careful in my js since i'm just reusing those elements.
Anyway, have a look and drop your comments in my work^^. I will also create the spock version
@DrKlonkPosted over 3 years agoHi Raymart!
Reaaaaaally well done on this one. Looks good, works good. A nice example for everyone to follow.
Particularly, I like the little animation on the 'rules' text and the fact that you don't get points reducted when you lose.
A minor thing I found in the scss: I think technically the flex classes like
flex--j-betweenare utility classes and not variables. I'd expect a _variables.scss to contain only scss variables. But maybe that's just me. You could check out the 7-1 pattern to structure SCSS. I really like it.Just one minor thing I found in the code: in displayResult() the
winneralways gets set toplayerChoice, although I don't think it affects anything.Again, very well done!
Cheers, Joran
1 - @wisniewskizSubmitted over 3 years ago
I used parcel bundler to compile all of my sass into a distribution ready file.
I'm not sure if I went about the cleanest or easiest way to create the layout using grid and flexbox, but the end results are useable. If you see anyway that I could have wrote more concise code please let me know!
This is my first solution to submit and I am very exited to learn and grow with everybody here.
@DrKlonkPosted over 3 years agoHi Zach!
Nice job for your first challenge! Don't be misled by the word newbie for this one, it is quite tricky. The desktop version looks nice!
Main thing: responsiveness I see some problems when I resize the screen to a mobile width, it kind of breaks. It works at 376px and lower, but everything in between is a bit wonky. If I resize the window, I get horizontal scrolling. And if I resize within devtools, at 775px for instance, the paragraph text size is too small because of the automatic resizing. On tablet sizes, the background image get pushed to the left, eventually going past the left side of
hero__ctaand the screen.I think these issues are caused by the grid that can't fit the screen.
Random other things
- You can add some line-height to the paragraph to gice it some space between lines.
- There are no error messages for the email input.
- It would also be nice if the google podcasts logo was centered vertically. Nothing a little flexbox can't fix.
If you like, you can check out my solution for this challenge as well, it can at least help if you want to make the error messages.
Don't be discouraged by my comments, you already took a very good step to join the community. Just trying to help!
Keep on coding! You'll just get better and better.
Cheers, Joran
1 - @saurabhkacholiyaSubmitted over 3 years ago
- Can anyone look into the code and see if my structuring of code is up to the standard for HTML and CSS?
- Can you please rate my design skill and point out areas of improvement?
- Is there any better way than the code has been written any blog or post to follow to improve CSS?
@DrKlonkPosted over 3 years agoHi Saurabh,
Decent work! The site resizes properly and I like that there is some feedback on clicking buttons (even though it is not that meaningful).
Some things to consider: Meta:
- Please use a separate file for your css. It is bad practice to put everything in one file. You can import the css file in your
<head>. - The class names are not consistent enough. Sometimes you do use the elements name (section/span/etc), sometimes you don't. Try to make the class names represent what the CSS is about. If the CSS is specific for the footer, call it 'footer'. If it is more of a utility class that gets reused, call it 'info-text-left' or something like that. Reusing a class named 'grow-together' for the same thing is a bit weird.
- Do fix the html and accessibility errors generated by Frontend Mentor
- I always like a
cursor: pointeron buttons.
Design:
- The text on nearly all buttons is quite tiny. Go for at least 14px (or 0.875rem).
- The top section needs more breathing room. It is quite different from the design in font size and margin of the text.
- The big image with the colored dots looks stretched in the vertical direction. You can just remove the
heightproperty to fix this. - The headers from "Grow together" and such are also too small. Compare it with the design and see the difference.
- You should hardly ever
text-align: centera paragraph of text. It makes it hard to read. Check the design here as well, it is different. - The "ready to build" part actually has a bit too much space in my opinion.
- The wavy image near the bottom has some whitespace on its bottom on some screen widths for me in Chrome. I can't find out why, but it shouldn't be there. It might be because in the HTML the
<img>is just floating there in between the main and the footer. You could make it the background of a before pseudo element of the footer, maybe that helps. Removing thewidth: 100%also removes the whitespace, but the result might not be what you want. - In the footer I'd expect the subscribe button to also trigger the alert but it didn't.
A lot of stuff! It's not all as important, but definitely enough room to improve.
Happy coding!
Cheers, Joran
0 - @Vitor-Silva27Submitted over 3 years ago@DrKlonkPosted over 3 years ago
Hi Vitor,
I like it! There are some minor things to improve, but overall it works nicely.
I somehow didn't even know that creating the
<input>inside of the<label>element was a thing. Mozilla shows it as an alternative, so it seems like a completely valid option. I asked a question in Slack#best-practices as to what is the preferred method.You can also toggle a class on an element in javascript with element.classList.toggle('.className'). I think that would lead to some cleaner code in your solution.
However, I think I prefer to handle the dark theme switch method described here. That saves adding classes to all items and writing those classes separately in the CSS.
In the smaller cards all the arrows are green and point upwards. Easy fix!
All in all, a great job!
Cheers, Joran
0 - @VbanetySubmitted over 3 years ago@DrKlonkPosted over 3 years ago
Hi Vinicius,
I like it a lot! The responsiveness is on point and it all looks nice.
Some minor details:
- On smaller screens, the headings of the cards could use some room to breathe on the top.
- The paragraphs of text in the cards should be
<p>instead of<span>. - The classless div inside of the
get-starteddiv could be removed. It also lacks padding when resizing (check 346px for instance) - The links in the footer should semantically be a list, I think
- You have some purposefully empty columns in the footer grid, for spacing. I think that is not that nice. Also, on 983px wide, the column for the first set of links is much smaller than for the second. I would not expect that. I think that extra space is for positioning the social icons properly, but that could be done without the empty columns too. You could also position the items inside the social column differently to fix this.
- The
<picture>tag is mainly used for multiple responsive images. I don't think it is needed here.
Again, it may look like a lot, but they are all quite minor and the page works as it should.
Cheers, Joran
1 - @seniorteckSubmitted over 3 years ago
after spending a couple of days trying to style the body with background property. I finally figured I was styling it in the wrong way I think, so I created a div with an empty element for the background to make it work, I don't know if this is the best practice, I used HTML & CSS for the project, would have loved to use sass but I am still a beginner.
any thought on how I can improve better on my code. pls would love to hear it. thank you
@DrKlonkPosted over 3 years agoHi AbdulKareem,
I would also check out the accessibility issues in the generated report. Those are quite relevant.
Design wise, the biggest thing for me would be the font and the way the text is styled. Check out here how to add a font to your page.
The padding on the paragraph makes it off line with the heading, which looks strange. I'd just remove the padding.
Also, your background is a bit darker than the design, making for less contrast on the paragraph, which makes it a bit more difficult to read.
In the code you use px and em for sizes a lot, I think it's well worth looking into using rems for all your sizes. It allows you to resize basically your entire site by setting the font-size in a media query. Check out this article on px vs rem, or do some googling on it.
Cheers, Joran
0 - @BeatiCodeSubmitted over 3 years ago
Please give feedback on my work. What can be improved?
@DrKlonkPosted over 3 years agoLooks good overall!
In the share button I see some room for improvement.
It would be nice to have it disable if you click outside of it as well.
It is also a bit weird that on smaller screens it covers the name and date (but maybe that was in the design).
In the code it is not great to have it dependent on the color of the button. I would probably toggle a class (button.classList.toggle('active')) on the button on click and fix everything in the CSS based on if the button has that class or not.
You could select the icon-social element in CSS with the "~" selector I think.
Also, it looks like you are tyring to use BEM for naming classes, but it's not consistent. If
title__contentis an element of the content block, it should becontent__title. After that you also use contact-user and userInfo, which are a bit ambiguous and use a different structure (hyphen and camelCase). Naming classes can definitely be tough, but can lead to much easier to read code. So it's well worth looking into! It's one of my own points of improvement as well.Cheers, Joran
Marked as helpful1 - @MarcoCarpoSubmitted over 3 years ago
Hi everyone, this is one of my first projects using React.js (still learning this). How does it look to you?
Also, does anyone have any idea how to remove the extra space above and below the text? (e.g. see the buttons, the text inside seems off-center due to these spaces) Is it possible to do this in Sass or do I need to apply changes directly to the font before importing it into the stylesheets? I mean, maybe I could implement a mixin that automatically sets the line-height based on the font-size. Or is this problem solved in another way? This is a issue that I often encounter and I still don't understand how to solve it. In any case, thanks for your attention!
@DrKlonkPosted over 3 years agoLooks good to me!
What do you mean with the extra space below the text? Which text(s) do you mean?
I think the buttons look a bit off centre because of 1) the font-family and 2) the fact that these words have little letters going down of the baseline (like g, p, or y for instance), making it a bit more obvious. It's a bit harder to get annoyed with the pixels in "Python" than it is in "Frontend", in my opinion.
Here's my font-family proof: If you change the font to monospace, the clickable labels look fine.
Fix for the smaller labels: On the labels like "NEW!" you could just add some more padding to the top in the job__feat class. I don't see much problem with that, except when you decide to change the complete font family, in which case you'd need to adapt at most 2 lines of CSS. No biggy.
Minor improvement: I'd also add a good old
cursor: pointer;to thejob__positionclass.Cheers, Joran
2
